BenChung / KRPC.jl

A kRPC client for Julia
MIT License
3 stars 4 forks source link

Enable asynchronous usage of KRPC #10

Closed Rhahi closed 2 years ago

Rhahi commented 2 years ago

This PR resolves two problems regarding async usage of KRPC.

Fix handling of parallel stream

First problem is that KRPC.jl previously did not allow parallel streaming. The problem comes from add_stream function:

  1. User calls add_stream
  2. handles are created: handles = kerbal(conn, ((AddStream_Phantom.(calls, true))..., ))
  3. listener is created listener = Listener(request_map, conn, default_value, out)
  4. the listener is remembered within a dictionary within KRPCConnection: setindex!.((conn.listeners, ), (listener, ), getproperty.(handles, :id))

Number 4 is where the problem occurs. Because the handle.id is unique for each type of stream (i.e., two parallel calls for universal time will have the same id), as soon as the second stream is established, the first one is forgotten and will never produce value (as the pump will not find it anymore)

This PR addresses the problem by assigning a unique identifier (instead of using handle id) whenever a new stream is created, resulting in this sequence:

  1. Assume that there was already a stream watching VAR1.
  2. User asks for a stream, watching for VAR1 and VAR2.
  3. a new UID is generated for this pair: uuid = uuid4()
  4. KRPCConnection remembers this listener with UID as a key: conn.listeners[uuid] = listener

So far so good, but this requires a further step because of handling of closing the streams. When closing stream for listener (VAR1 + VAR2), we can't just close the connection just yet. That's because streams are closed using the handle ID (push!(req, RemoveStream_Phantom(id)). When a connection for VAR1+VAR2 is removed, another stream that watches VAR1 will also be unavailable. Therefore we must remember that VAR1 should stay open until another listener for VAR1 wants to close, too.

  1. Create a one-to-many relationship between ID of the variable being watched, and UIDs.
  2. When a listener (associated with UID) wants to close, instead of closing it immediately, just remove it from the ID list.
  3. If the removed UID from an ID list is the final one, we are finally safe to remove the actual stream from KRPC.

Note: This will introduce a new dependency for Julia package UUIDs. Alternatively, having a mutable global (or in KRPCConnection) variable that always increments can also solve without introducing such dependency. If you prefer to have something like that, I can make a new PR with that.

Fix race condition when calling SendBiMessage

This problem will cause KRPC.jl to hang and eventually crash when a remote call is produced while one has not been completed. It is easier triggered if the user creates multiple async calls, each calling for some Kerbal values or commands. Initially, I attempted to fix it by wrapping the function with sync-async block, but this made no effect. I instead added a semaphore within KRPCConnection, so that SendBiMessage can synchronize using that.

BenChung commented 2 years ago

That's fantastic work!

Number 4 is where the problem occurs. Because the handle.id is unique for each type of stream (i.e., two parallel calls for universal time will have the same id), as soon as the second stream is established, the first one is forgotten and will never produce value (as the pump will not find it anymore)

Interesting - I thought a priori that the id was unique per stream, not per request. Good job noticing that.