Two identical streams are both deleted when calling RemoveStream on one of them

[Cahu]

Hey !

When making identical stream requests it appears the same stream id is returned and a single update is sent with each StreamUpdate instead of two. The behavior seems to be the same whether these requests are made in distinct packets or together in a batch.

For example, using the client I'm working on:

let stream1 = client.mk_call(&krpc_mars::mk_stream(&space_center::get_ut()))?;
let stream2 = client.mk_call(&krpc_mars::mk_stream(&space_center::get_ut()))?;
let update = stream_client.recv_update()?;
println!("Stream1: {:?}", stream1);  
println!("Stream2: {:?}", stream2);
println!("update: {:?}", update);

Produces:

Stream1: StreamHandle { stream_id: 41, ... }
Stream2: StreamHandle { stream_id: 41, ... }
update: StreamUpdate { updates: {41: value: "\211\355\013\207\006*\326@"} }

This could be a problem when those streams are created by independent parts of the client program and one of them decides it no longer needs the stream and removes it. When that happens, the stream disappears for both of them.

I believe this behavior is intended to minimize the amount of data sent to the client. Maybe a counter could be added server-side so that in the above scenario two calls to RemoveStream are necessary to completely remove the stream.

[lucaelin]

Most client libraries seem to have overcome this by wrapping streams into an "object", that keeps track of how many active listeners there currently are and only if all listeners are removed, the api call to remove the stream would be issued... But this requires a higher level interface to the api calls, so a low level api would indeed require support from the server...

[Cahu]

Yes, this could easily be done by the client and I wouldn't mind implementing it. I raised the issue because I think the overall direction taken by kRPC is to avoid features being developed multiple times in clients.

[djungelorm]

Apologies for the slow reply.

This is indeed done to save on network usage and improve performance. The client libraries I've written actually don't do reference counting (I verified this in the python client, and am pretty sure all the others work the same). When a stream is removed, all duplicates of the stream are also removed. I see how this could be confusing to the library user - and having the removal reference counted is probably better.

Doing the reference counting on the server would indeed be best - to avoid re-implementation efforts in each client. However, we should be careful to do it in such a way that existing versions of clients continue to work with the same behaviour though. Maybe add a boolean flag to KRPC.RemoveStream that indicates whether the stream should be removed immediately, or removed only when its reference count reaches zero? The default would be to remove immediately - so that old clients work the same as before.

[Cahu]

This looks like a good idea. Similarly we could add this flag in the AddStream request instead to indicate that we want the stream reference counted or, if it already exists, promote it to a reference counted stream. This alternative might not be as interesting as adding the flag in RemoveStream (easier cleanup in clients for example); I'm just wondering what would be the least surprising from an API standpoint.