WebTransport session #1252
WebTransport session #1252
Conversation
|
This is based on #1250 . The last 2 changes are new. This is still a draft, because I need a find a good way to remove ExtendedConnectSession from the HashTable in ExtendedConnectFeature. |
There was a problem hiding this comment.
Thanks for putting this together. I don't think that I was quite thorough enough with this review, but I've run out of time, so I'll let you have these commends and I'll take another look later.
I worry a little that the test coverage is a bit light given the amount of code involved here, but I need to think about it some more. The websockets question is an obvious one; there might be more.
neqo-http3/src/client_events.rs
Outdated
| pub enum WebTransportEvent { | ||
| WebTransportNegotiated(bool), | ||
| WebTransportSession(u64), | ||
| WebTransportSessionClosed { | ||
| stream_id: u64, | ||
| error: Option<AppError>, | ||
| }, | ||
| } |
There was a problem hiding this comment.
These look to be generic: they also apply to the server.
There was a problem hiding this comment.
The problem is that the server needs more than just stream/session_id. It also need connection. There are not completely the same.
neqo-http3/src/client_events.rs
Outdated
| @@ -142,6 +154,33 @@ impl SendStreamEvents for Http3ClientEvents { | |||
| } | |||
| } | |||
|
|
|||
| impl ExtendedConnectEvents for Http3ClientEvents { | |||
| fn extended_connect_session_established( | |||
There was a problem hiding this comment.
That's a very long name. Any reason not to name these session_start/session_end. There are other things that might use "session" as a name, but we can revisit the names if a collision comes up.
I would add "negotiated(HSettingType)" to this interface.
There was a problem hiding this comment.
negotiated has been moved, there is not a separate listener for it.
| @@ -733,6 +737,116 @@ impl Http3Connection { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub fn webtransport_create_session<'x, 't: 'x, T>( | |||
There was a problem hiding this comment.
Do the lifetime constraints need to be different than the ones you now have on RequestDescription?
There was a problem hiding this comment.
yes, because of some strings.
| /// Close sending side. | ||
| /// # Errors | ||
| /// It may return `InvalidStreamId` if a stream does not exist anymore. | ||
| pub fn stream_close_send(&mut self) -> Res<()> { |
There was a problem hiding this comment.
Is implementing Deref[Mut] easier than replicating all these functions?
|
I have open a separate issue for this comment . That will be resolve in a separate PR. Let's review this part it is big. |
The fetch will be used by the extended connect requests aas well.
This PR creates a generic ExtendedConnectFeature structure that is responsible for a feature negotiation and keeps a HashMap of the extended connect requests. The HashMap will be used for finding active sessions. An extended connect request will be created as a regular fetch request only the listener for the events will be different. A generic ExtendedConnectSession structure is created for each extended connect request and is responsible for listening to request events and following the state of the request/session, e.g. the session is being negotiated, a stream is closed, etc..This structure will be responsible for bookkeeping, i.e. it will hold information about active streams open on a session and it will make sure to close all streams if the session request is closed. Currently ExtendedConnectSession cannot read data from the request stream, this will be fixed in a separate PR. On the server side, upon receiving headers a regular http request will be transformed into an extended connect request and a new ExtendedConnectSession will be set to be the listener for the request. Regular http request functions will be used for closing an extended connect request, e.g. steam_stop_sending, cancel_fetch, etc. Additional changes: Now it is not easily possible to distinguish between Http stream and Push stream, therefore RecvMessage holds the type of the stream. WebTransport’s extended connect request will be created as a normal fetch request. The listener for the events on this request will be different All WebTransport events are put into a separate enum structure.
This PR creates a generic ExtendedConnectFeature structure that is responsible for a feature negotiation and keeps a HashMap of the extended connect requests. The HashMap will be used for finding active sessions.
An extended connect request will be created as a regular fetch request only the listener for the events will be different. A generic ExtendedConnectSession structure is created for each extended connect request and is responsible for listening to request events and following the state of the request/session, e.g. the session is being negotiated, a stream is closed, etc..This structure will be responsible for bookkeeping, i.e. it will hold information about active streams open on a session and it will make sure to close all streams if the session request is closed.
Currently ExtendedConnectSession cannot read data from the request stream, this will be fixed in a separate PR.
On the server side, upon receiving headers a regular http request will be transformed into an extended connect request and a new ExtendedConnectSession will be set to be the listener for the request.
Regular http request functions will be used for closing an extended connect request, e.g. steam_stop_sending, cancel_fetch, etc.
Additional changes:
Now it is not easily possible to distinguish between Http stream and Push stream, therefore RecvMessage holds the type of the stream.
WebTransport’s extended connect request will be created as a normal fetch request. The listener for the events on this request will be different
All WebTransport events are put into a separate enum structure.