fix(client): disconnect_reason/read_error is cancel-safe
#1347
Conversation
If/when the connection is closed, the cause is fetched by `read_error` from the background task. It was not cancel-safe, which could have side-effects and mutate the state, such as the internal Option could be `None` and cause a panic.
core/src/client/async_client/mod.rs
Outdated
| }; | ||
|
|
||
| Error::RestartNeeded(err) | ||
| if let Some(err) = self.disconnect_reason.read().await.as_ref() { |
There was a problem hiding this comment.
Now we're not holding this lock over any await points (at least, I think we aren't?), should we make it a sync RwLock?
There was a problem hiding this comment.
done nice catch, one dependency removed :)
Actually, we have https://docs.rs/jsonrpsee-ws-client/latest/jsonrpsee_ws_client/struct.WsClientBuilder.html#method.request_timeout where the "future/method call" is dropped and it may suffer from this cancel-safety issue and "nothing caused by the user". Thus, an internal jsonrpsee issue as which I forgot about... But it could be caused |
* client: `disconnect_reason/read_error` cancel-safe If/when the connection is closed, the cause is fetched by `read_error` from the background task. It was not cancel-safe, which could have side-effects and mutate the state, such as the internal Option could be `None` and cause a panic. * fix wasm build * remove async_lock dependency
|
I had a fix in different way, it uses |
Yepp, that works as well but I didn't like the old code and decided to refactor it. The RwLock is basically only "write_locked once" and should be quite efficient anyway because once the connection is closed it should never block (only read locks can be held) I guess we could change it to a OnceLock instead but I'm quite happy with this for now |
Bump jsonrpsee to fix paritytech/jsonrpsee#1347
If/when the connection is closed the cause is fetched by
read_errorfrom the background task. It was not cancel-safe which could have side-affects and mutate state such the internal Option could beNoneand end-up in a panic.