fix: tokio v1.27 #1062
fix: tokio v1.27 #1062
Conversation
| fn poll_stop_monitor_heartbeat(&mut self, cx: &mut Context) { | ||
| // We don't care about the ticks of the heartbeat, it's here only | ||
| // to periodically wake the `Waker` on `cx`. | ||
| let _ = self.stop_monitor_heartbeat.poll_tick(cx); |
There was a problem hiding this comment.
IIUC we relied on this to make sure that we wake up:
- when there's no scheduled tasks that would yield to the executor (call waker.wake())
- for at most one second
To remove tokio::time::Interval we are polling the FutureDriver alongside other operations that we do in the server:
- try_accept_conn: Await incoming connection
- wait_for_permit: Await backpressure for some internal buffer space
- try_recv: Can wait for
soketto::Pong,soketto::Data, andsoketto::Closed
Could we still get in a situation where we have no other competing tasks to drive the FutureDriver?
(ie, we don't have soketto::Pong to wake us up -- or is configured at 100seconds -- and we don't receive soketto::Data)
There was a problem hiding this comment.
After convincing myself that this DriverSelect thing (ie select_with) will poll the futures in itself as needed, maybe I'm missing something but why does it matter if this thing doesn't wake up for a while if none of the futures in it need to progress?
There was a problem hiding this comment.
I was concerned that the FutureDriver will not be polled on time to handle a Ready future from its internal vector. Meaning that if we don't get enough traction from the try_recv, we previously made sure to have some progress with the tokio::Interval.
I was trying to imagine the unlikely scenario where soketto::recv from try_recv won't generate events for 10 minutes, but our futures from the driver are all ready. In this case, the tokio::Interval was making sure to advance things a bit sooner.
There was a problem hiding this comment.
My current understanding is now that when any of those internal futures makes progress, the driver task will be polled again (or whatever task contains the Driverselect, anyway) and they will all re-run. So, no futures in this list should be ignored, basically. If an internal item is Ready, then it'll have called wake() to make everything get polled again.
There was a problem hiding this comment.
so, that's a good question/observation but I found that quite unlikely because both batch and calls are executed as one future/task.
Then in each loop iteration both try_recv and wait_for_permit are awaited on which checks "the future vec" but it could be possible to spawn huge futures and then "never send some data again/backpressure kicks in" then those would never woken up again but I don't see it as an issue because it's up to the client enforce that.
If it's idle fine but we could have a few ready tasks in the future vec.
| @@ -167,7 +168,7 @@ where | |||
| } | |||
| } | |||
|
|
|||
| connections.await; | |||
| while connections.next().await.is_some() {} | |||
There was a problem hiding this comment.
Ah ok, so if I understand this right, FuturesUnordered won't poll anything until you tell it to, so you are spawning tasks into it (so that they can progress independently) and then this line just makes sure that all tasks are completed before the function ends?
(I think that seems perfectly reasonable to me! Perhaps worth a comment)
* fix: tokio v1.27 * Update server/src/transport/ws.rs * fix rustdoc * Update server/src/transport/ws.rs * Update server/src/transport/ws.rs * no more futuredriver for incoming conns * add comment for unclear code
* IntoResponse trait for rpc calls * remove ErrorResponse * cleanup, fix nits * separate types from ser/deserialization * fix uncommented code * add Success type * add missing dev-dep * fix tests with issue link * fix tests * add missing file * PartialResponse -> ResponsePayload * chore(deps): bump actions/checkout from 3.4.0 to 3.5.0 (#1055) Bumps [actions/checkout](https://github.com/actions/checkout) from 3.4.0 to 3.5.0. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3.4.0...v3.5.0) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump baptiste0928/cargo-install from 1 to 2 (#1056) Bumps [baptiste0928/cargo-install](https://github.com/baptiste0928/cargo-install) from 1 to 2. - [Release notes](https://github.com/baptiste0928/cargo-install/releases) - [Changelog](https://github.com/baptiste0928/cargo-install/blob/main/CHANGELOG.md) - [Commits](baptiste0928/cargo-install@v1...v2) --- updated-dependencies: - dependency-name: baptiste0928/cargo-install dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix: tokio v1.27 (#1062) * fix: tokio v1.27 * Update server/src/transport/ws.rs * fix rustdoc * Update server/src/transport/ws.rs * Update server/src/transport/ws.rs * no more futuredriver for incoming conns * add comment for unclear code * ResponsePayload Cow-like * fix ugly code * cleanup * address grumbles * ToOwned -> Clone * compile-time tests to workaround rustc bug * Update proc-macros/src/helpers.rs * add missing impls for Vec and [T; N] --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Tokio v1.27 changed how interval/timers are woken up.
Because we already have
async fn StopHandle::shutdownwe could just select on that along with the other future.Thus, these periodically wake-ups are not needed anymore which should fix this properly.
Close #1059