Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update quinn and rustls #320

Merged
merged 1 commit into from
Oct 29, 2023
Merged

Update quinn and rustls #320

merged 1 commit into from
Oct 29, 2023

Conversation

patowen
Copy link
Collaborator

@patowen patowen commented Oct 28, 2023

Update rustls from 0.20.6 to 0.21.7.
Update quinn from 0.8.3 to 0.10.2.

Most changes are simple compatibility fixes, mostly involving turning previously-quinn-provided streams into loops with task spawns.

One small but potentially important change is that the ClientEvent::Lost message will always be sent when an error occurs, even if the connection was already closed from some other logic and the client was already cleaned up. To be robust to this kind of redundancy and potential race conditions, the on_client_event method is set up to skip messages received from clients that were already cleaned up.

@patowen patowen requested a review from Ralith October 28, 2023 00:18
client/src/net.rs Outdated Show resolved Hide resolved
@Ralith
Copy link
Owner

Ralith commented Oct 28, 2023

The current naive tokio::spawn-based handling of drive_recv allows client messages to appear for a given client after ClientEvent::Lost

One strategy to avoid this might be to only generate Lost messages after all tasks associated with a client have ended. Alternatively, we could use some shared state (perhaps quinn::Connection::close_reason()) to judge that we should drop messages rather than sending them on.

certain convenience methods like buffer_unordered are no longer available. This has resulted in an increased number of tokio::spawn calls, and I haven't thought them out well.

This is the correct refactoring. Concurrency limits are built-in to quinn now.

@patowen
Copy link
Collaborator Author

patowen commented Oct 28, 2023

One strategy to avoid this might be to only generate Lost messages after all tasks associated with a client have ended. Alternatively, we could use some shared state (perhaps quinn::Connection::close_reason()) to judge that we should drop messages rather than sending them on.

I like the idea of delaying the ClientEvent::Lost message, as the more data the server receives from the client, the more graceful their exit can be. Now that I know this approach I'm taking doesn't seem too far off, I'll give this another shot.

This is the correct refactoring. Concurrency limits are built-in to quinn now.

Oh, nice! Good to know there are fewer constants we have to configure.

@patowen
Copy link
Collaborator Author

patowen commented Oct 28, 2023

Thinking about the Lost-messages problem more, the client can be removed for any reason, and I hope I can find a solution to this problem that doesn't require being too careful in the future. Fortunately, there's a feature of slotmap that I was unaware of:

The keys returned by slotmap are versioned. This means that once a key is removed, it stays removed, even if the physical storage inside the slotmap is reused for new elements. The key is a permanently unique* reference to the inserted value.

Therefore, I believe the main solution here is simply to have on_client_event ignore messages for non-existing client IDs. The asterisk I believe is just due to numerical overflow, but that shouldn't be an issue here.

There's still the question of how to gracefully handle client exits, but that feature doesn't exist in Hypermine yet in the master branch (clients just quit, and servers say "dropping slow client"), so I don't need to solve that in this PR, since its purpose is to just update quinn.

Anyway, I should be able to update this PR with the help you gave, hopefully enough to be a non-draft.

@patowen patowen marked this pull request as ready for review October 28, 2023 17:08
@patowen patowen requested a review from Ralith October 28, 2023 17:13
server/src/lib.rs Outdated Show resolved Hide resolved
@patowen patowen force-pushed the update-quinn branch 2 times, most recently from 77a9926 to 123efdd Compare October 28, 2023 21:17
@patowen patowen requested a review from Ralith October 28, 2023 21:19
client/src/net.rs Outdated Show resolved Hide resolved
server/src/lib.rs Outdated Show resolved Hide resolved
rustls: 0.20.6 -> 0.21.7
quinn: 0.8.3 -> 0.10.2
@patowen patowen merged commit 20b79d4 into Ralith:master Oct 29, 2023
6 checks passed
@patowen patowen deleted the update-quinn branch October 29, 2023 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants