fix(async): handle close of packet stream to avoid panic

[sirkrypt0]

When the packet stream closes, e.g. because the server terminated the connection, the stream returns None. Then, the next() function shouldn't be called again on the stream, as it is already closed.
Previously, we wouldn't handle this, which lead to a panic.

Before the fix which implemented the as_stream() function for the socket, the program wouldn't panic, but instead run in an endless loop which isn't ideal either.

Now, the loop which fetches the packets from the stream properly terminates. However, the program which uses the socket.io client currently isn't notified of the unexpected closure.

Closes #323

While I tried writing a test for this, it appears that you can't assert whether some tokio thread panicked or not without having the handle to that thread (there is a feature in tokio_unstable called unhandled_panic though).
Since the builder currently spawns the stream iterator without returning a handle, I didn't find a way to properly assure the this thread doesn't panic.

[codecov]

Codecov Report

Merging #324 (f95241d) into main (cb107ba) will increase coverage by 0.06%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #324      +/-   ##
==========================================
+ Coverage   90.94%   91.01%   +0.06%     
==========================================
  Files          37       37              
  Lines        4673     4672       -1     
==========================================
+ Hits         4250     4252       +2     
+ Misses        423      420       -3     

Impacted Files	Coverage Δ
socketio/src/asynchronous/client/builder.rs	93.51% <66.66%> (+0.06%)	⬆️

... and 3 files with indirect coverage changes

[1c3t3a]

First of all thanks for the PR! I appreciate your contributions to the async version of the crate a lot, the stream impl isn't perfect, but IMO currently the best to do in async rust.

While I tried writing a test for this, it appears that you can't assert whether some tokio thread panicked or not without having the handle to that thread (there is a feature in tokio_unstable called unhandled_panic though).

Do you think it makes sense to add this feature for tests only? That case we can verify to not brake it and only add a dependency for devs and not for normal users of the crate.

However, the program which uses the socket.io client currently isn't notified of the unexpected closure.

Yes, and this is an issue (also see the opened issues around it). Do you have an idea how to fix this (e.g. invoke the err callback or something else?). We can't invalidate a Client that we already handed out nicely from a background thread anyways, so it is rather about informing the user properly about what is going on.

[sirkrypt0]

Do you think it makes sense to add this feature for tests only? That case we can verify to not brake it and only add a dependency for devs and not for normal users of the crate.

I totally agree that it would make sense to test this and tried it out here https://github.com/giz-berlin/rust-socketio/blob/fix/async-stream-close-test/socketio/src/asynchronous/client/client.rs#L555-L603
Sadly, enabling tokio_unstable for tests only is rather hacky since it's not a feature but a cfg and apparently you cannot do the following in .cargo/config.toml.

[target.'cfg(test)']
# Rustflags specifically for tests
rustflags = ["--cfg", "tokio_unstable"]

See rust-lang/cargo#8170.
[target.'cfg(debug_assertions)'] to only enable it on debug builds doesn't seem to work either.

Instead I enabled it via the RUSTFLAGS environment variable in the CI...
We may make use of the Makefile here though, which is currently not used in the CI I think and set the RUSTFLAGS there.
Then, it would at least be consistent for the CI and for users which run make test....

Let me know what you think :)

[sirkrypt0]

Yes, and this is an issue (also see the opened issues around it). Do you have an idea how to fix this (e.g. invoke the err callback or something else?). We can't invalidate a Client that we already handed out nicely from a background thread anyways, so it is rather about informing the user properly about what is going on.

Calling the error callback would be possible. However, a limitation I currently see with the callbacks is that they are limited to the Payload which is either Bytes or a String. So determining the actual error type would come down to error-prone string comparison I think.

I think the use of the error callback is currently mixed between errors coming from the server and errors occurring locally. For example, the ConnectError packet type is handled by calling the error callback and concatenating an error message with the actual payload but any local errors are handled by calling the error callback as well.

rust-socketio/socketio/src/client/raw_client.rs

Lines 377 to 386 in 39297a5

	PacketId::ConnectError => {
	self.callback(
	&Event::Error,
	String::from("Received an ConnectError frame: ")
	+ &packet
	.clone()
	.data
	.unwrap_or_else(|| String::from("\"No error message provided\"")),
	)?;
	}

So maybe, the on callbacks should be used for events which are sent across the SocketIO connection only and be always passed the packet payload.
Then, for any errors which occur locally (probably all of the socketio::Error enum) a separate .on_error callback may be used which gets the actual error? Although it is questionable whether users can actually do useful things when they can distinguish the exact error type like InvalidUtf8 or InvalidPoisonedLock (as it says in the TODO: Do not expose non-trivial internal errors).

Additionally, it may be useful to have a separate on_disconnect callback which is called when a disconnect (either normal closure via an Event::Close or unexpected closure) occurs to make the disconnection explicit to the user. This would avoid that users have to parse the error to decide whether an unexpected disconnect occurs?
It could be implemented in a similar fashion to https://socket.io/docs/v4/client-api/#event-disconnect which provides a reason for the disconnect.

[1c3t3a]

Instead I enabled it via the RUSTFLAGS environment variable in the CI...
We may make use of the Makefile here though, which is currently not used in the CI I think and set the RUSTFLAGS there.
Then, it would at least be consistent for the CI and for users which run make test....

+1 This sounds like a feasible approach, can you maybe create an issue for when to remove it and add a doc comment? I think it's just good if those tests actually run.

I think the use of the error callback is currently mixed between errors coming from the server and errors occurring locally. For example, the ConnectError packet type is handled by calling the error callback and concatenating an error message with the actual payload but any local errors are handled by calling the error callback as well.

Yeah this isn't optimal. I think parsing the error is the worst approach to take here, so the on error callback shouldn't be the thing to use.

So maybe, the on callbacks should be used for events which are sent across the SocketIO connection only and be always passed the packet payload.
Then, for any errors which occur locally (probably all of the socketio::Error enum) a separate .on_error callback may be used which gets the actual error?

I agree with you here. Maybe we can split the error enum to errors that we want to expose and internal errors that don't matter to the user but only to us.

Also +1 to the on_disconnect callback, sounds reasonable.

I hope to find time to work on this the upcoming weeks, if not, feel free to pick something up! :) And thanks for the your comments here!