feat(socketio): add ability to wait for socketio completion

[sirkrypt0]

When the async client connects to the server, it spawns a new thread to handle the arriving messages asynchronously. Previously, this thread was immediately detached with no chance of awaiting its completion.

For long-running programs (e.g. a client program that never really disconnects from the server) this can be problematic, as unexpected stream completions would go unnoticed. This may happen if the underlying tungstenite websocket shuts down ('Received close frame: None') but there are no engineio/socketio close frames. Hence, since the stream terminates, the message handling task stops without a Close or Error event being fired.

Therefore, we now provide a new wait_for_completion function on the socketio client which allows to wait until the polling stream has terminated. Programs can then use this to determine whether an unexpected shutdown has happened and take further actions on their own.

[felix-gohla]

While it works, I do not really know whether this makes the API design better.

It feels a little janky to have that wait_for_completion function while the client doesn't really have a notion of completion. I am thinking of alternative and ergonomic ways of using the client.

One should be able to:

• manually disconnect
• send messages
• receive messages
• receive an error and forcefully tell the user of the client about that

Without calling the wait_for_completion function, you wouldn't know that there was an error, ay?

[felix-gohla]

I think this should return some kind of result (like an error reason?), as it can fail in various ways:

• Client was never connected
• Client did successfully disconnect itself (graceful shutdown requested).
• Server sent an error or whatever.

[sirkrypt0]

Client did successfully disconnect itself (graceful shutdown requested).

We could make the DisconnectReason public and return it here.

Server sent an error or whatever.

That's handled through .on(Event::Error.

Client was never connected

We could return a Result which contains an error if the client was not connected and the disconnect reason otherwise.

[felix-gohla]

Then why don't handle the server-side disconnect via the on callbacks and make this poll function blocking? I see some kind of split API here and I feel like not knowing which method to use when.

[sirkrypt0]

It feels a little janky to have that wait_for_completion function while the client doesn't really have a notion of completion.

If the polling task is completed, the client is done, until you call poll_stream again of course. We could also rename it, e.g. wait_for_task_completion, wait_for_poll_completion, wait_for_termination, ...

Without calling the wait_for_completion function, you wouldn't know that there was an error, ay?

Depends on what you define as error. You do receive application errors from socket.io or engine.io through the Event::Error, but previously the termination of the polling task was not propergated, so you wouldn't know when the websocket stream closed properly but without any engine.io / socket.io close messages.
So in typical scenarios, where e.g. the server would shut down or the network breaks or whatever, where we don't receive any responses anymore, we would get notified. Just not in this weird case where the underlying websocket closes properly but without any engine.io / socket.io frames (or probably when the poll task panics).

[felix-gohla]

If the polling task is completed, the client is done, until you call poll_stream again of course.

This is also a little weird. Shouldn't I call connect or something else that notifies that it actually does something instead of the non-descriptive poll_stream? What does this mean anyways? This poll function is only called once, so you're not doing polling when calling the function.

But this aside, as said in the discussion above, not sure whether the additional function is the way to go. 😊

[felix-gohla]

I think closing the upper stream is a better option. Thank you. :)

[sirkrypt0]

Submitted to upstream.