fix: ws server terminate subscriptions when connection is closed by the client. #483
Conversation
| @@ -241,7 +241,13 @@ async fn background_task( | |||
| while !stop_monitor.shutdown_requested() { | |||
| data.clear(); | |||
|
|
|||
| method_executors.select_with(receiver.receive_data(&mut data)).await?; | |||
| if let Err(e) = method_executors.select_with(receiver.receive_data(&mut data)).await { | |||
There was a problem hiding this comment.
NOTE; this will kill a connection on any soketto::connection::Error, not just if the client terminated the connection
In a follow-up PR I think we should try to do some graceful error handling
niklasad1
changed the title
niklasad1
changed the title
niklasad1
changed the title
tests/tests/integration_tests.rs
Outdated
| // assert that the server received `SubscriptionClosed` after the client was dropped. | ||
| assert!(rx.next().await.is_some()); |
There was a problem hiding this comment.
This is a bit opaque unless one knows exactly what is going on.
Maybe we can do something like this?
let sub_closed_err = rx.next().await.expect("Server received `SubscriptionClosed` after the client was dropped.");
assert!(format!("{:?}", sub_closed_err).contains("is closed but not yet dropped"));
It's a bit annoying that SubscriptionClosedErr has a private member (and that there's no way for the client to know what the subscription ID is) or we could have used assert_matches here for better readability...
There was a problem hiding this comment.
Yepp, I think we could replace SubscriptionClosed(String) with a better type.
Perhaps with a kind enum, to distinguish why the subscription was closed the error message is quite useful atm
Closing #481
The underlying issue is that
receiverof the connection channel is not dropped when the connection is terminated and the correspond senders are stored in the subscription HashMap.With this change the
receiveris dropped when the connection is terminated and whenSubscriptionSink::sendis tried it will fail and cause the server the remove the subscription.