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

Make server::Connection methods not require &mut self #186

Open
ten3roberts opened this issue Apr 6, 2023 · 4 comments
Open

Make server::Connection methods not require &mut self #186

ten3roberts opened this issue Apr 6, 2023 · 4 comments
Labels
B-rfc Blocked: request for comments. Needs more discussion. C-refactor Category: refactor. This would improve the clarity of internal code.

Comments

@ten3roberts
Copy link
Contributor

Currently, many methods in server::Connection require exclusive access, which makes it nigh impossible to concurrently use the connection.

This is not a large problem at the moment, as the only function of interest is the Connection::accept method.

However, when adding support for the H3_DATAGRAM protocol it is not possible to await datagrams and accept requests on the same connection.

I ran into this issue when implementing webtransport in #183, which requires datagram support.

In addition, webtransport has support for multiple sessions per connection, which in order to support requires that the connection can be used both for accepting webtransport datagrams and streams and accepting new requests such as CONNECT.

Simply Arc<Mutex<Connection>> does not solve the issue as the MutexGuard would span an await point on accept.

Making Connection::accept be async fn accept(&self) -> ... would solve the issue. Quinn has a similar api for their accept apis https://docs.rs/quinn/latest/quinn/struct.Connection.html

Is this a change that we are interested in adding? If so, I could work on it.

@inflation
Copy link
Contributor

inflation commented Apr 6, 2023

quinn's Connection uses Arc<Mutex> under the hood, so if you're holding MutexGuard now, you probably will hold the guard when locking internal states too.

@Ruben2424 Ruben2424 added C-refactor Category: refactor. This would improve the clarity of internal code. B-rfc Blocked: request for comments. Needs more discussion. labels Apr 6, 2023
@seanmonstar
Copy link
Member

So, I just wanted to start by bringing in the original design proposal, as it says this:

Synchronization: As much as possible, synchronization should not be an internal feature. We’ve learned from building the h2 crate, which includes a Mutex around the stream store and buffer, and many different stream handles. This adds contention, and makes it increasingly difficult to realize when a stream is no longer reachable.

That was the goal. Is it possible to come up with reasonable solutions that don't compromise? I don't mean to ask for ridiculous workarounds, but to just make sure reasonable ones are explored.

For instance, later in that proposal, a trait BidiStream was proposed, providing QUIC implementations a way to opt-in to a stream.split(). But h3 wouldn't require it be implemented, if the QUIC library couldn't cheaply do so.

@ten3roberts
Copy link
Contributor Author

That is a good point.

Would it be possible to remake the accept into a poll_accept and a wrapper, so that the user can write a poll style function which only briefly locks the guard?

This would allow for simultaneous webtransport and request handling, which is necessary to support more than one CONNECT session

@Ruben2424
Copy link
Contributor

Ruben2424 commented Apr 11, 2023

Would it be possible to remake the accept into a poll_accept and a wrapper, so that the user can write a poll style function which only briefly locks the guard?

I think this is not necessary to completely remake accept because it calls poll_accept_request where all the simultaneous waiting happens right now.
Then poll_accept_request may need a other name.

let mut stream = match future::poll_fn(|cx| self.poll_accept_request(cx)).await {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-rfc Blocked: request for comments. Needs more discussion. C-refactor Category: refactor. This would improve the clarity of internal code.
Projects
None yet
Development

No branches or pull requests

4 participants