Skip to content

Introduce new stream and multiplexed bedrock types #17213

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

Merged
merged 27 commits into from
Feb 25, 2020

Conversation

jkotalik
Copy link
Contributor

@jkotalik jkotalik commented Nov 19, 2019

Fixes #17037.

Implements the following:

public class MultiplexedConnectionContext : ConnectionContext
{
    ValueTask<StreamConnectionContext> AcceptAsync(CancellationToken token = default);
    ValueTask<StreamConnectionContext> ConnectAsync(IFeatureCollection features = null, bool unidirectional = false, CancellationToken token = default);
}
public class StreamConnectionContext : ConnectionContext
{
    long StreamId { get; } // ConnectionId is a string today, StreamId for HTTP/3 needs to be a long, figure out how to consolidate.
    Direction Direction{ get; }
}
public enum Direction
{
   BidirectionalInbound,
   BidirectionalOutbound,
   UnidirectionalInbound,
   UnidirectionalOutbound
}
public class IConnectionFactory
{
   ValueTask<ConnectionContext> ConnectAsync(Endpoint endpoint, IFeatureCollection features = null, CancellationToken token = default);
}

A majority of this right now is generated code, which probably can be improved. Right now, the generated code needs to be a separate class per context type (Multiplexed, Stream, and Connection) because each of the Transport classes derive from those. For example:

internal abstract partial class TransportMultiplexedConnection : MultiplexedConnectionContext

@jkotalik
Copy link
Contributor Author

@davidfowl @anurse @halter73 thoughts?

@jkotalik jkotalik force-pushed the jkotalik/multiplexedFactory branch from 62d1880 to f9b88b7 Compare December 11, 2019 18:55
@jkotalik
Copy link
Contributor Author

I spent the last few hours trying to update this PR to "split the world". Going to take a small break because it honestly was a lot to juggle here and I need to take a fresh look. Few open questions:

  • How much value is there to duplicate HttpConnection and HttpConnectionContext. Right now, I put both the MultiplexedConnectionContext and ConnectionContext there, but there may be value in doing the protocol selection inside of the middleware rather than the HttpConnection class.
  • We talked about potentially have a base class for MultiplexedConnectionContext and ConnectionContext. Ex the ConnectionLimiterMiddleware may be something to share.
  • There is still a lot of work to do with the ConnectionDispatcher and KestrelServer.

@analogrelay
Copy link
Contributor

Poke @jkotalik

@jkotalik jkotalik force-pushed the jkotalik/multiplexedFactory branch from 777beb2 to 3894d70 Compare February 13, 2020 23:48
@jkotalik jkotalik marked this pull request as ready for review February 18, 2020 19:11
@jkotalik jkotalik requested a review from a team February 18, 2020 19:11
@davidfowl
Copy link
Member

cc @scalablecory

@jkotalik
Copy link
Contributor Author

Alright, this PR got a little large, so let me break it down into pieces.

This PR consists of:

  • Adding interfaces IMultiplexedConnectionListener, IMultiplexedConnectionBuilder, IMultiplexedConnectionFactory, and IMultiplexedConnectionListenerFactory for multiplexed connections/streams
  • Add StreamContext which derives from ConnectionContext and MultiplexedConnectionContext.
  • Adapts HTTP/3 to use the new multiplexed factory. This includes adding Http3(Stream/Connection)Context, a MultiplexedConnectionManager, MultiplexedKestrelConnection, etc. Some of these types are very similar to each other; I think I can still refactor this to use a new base class between ConnectionContext and MultiplexedConnectionContext, however it's a bit tricky.
  • Adds a bunch of HTTP/3 tests (most of these are unrelated, but mostly for sanity checking).
  • Fixing a few various bugs, mostly copying/porting from HTTP/2.
  • Allows stream/connection to set protocol error code on underlying quic stream/connection.

@@ -3,10 +3,9 @@

namespace Microsoft.AspNetCore.Connections.Features
{
public interface IQuicStreamFeature
public interface IStreamDirectionFeature
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know if we wanted these to be on the StreamContext yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think StreamContext is different enough from ConnectionContext to justify its existence. Maybe this could be added to ConnectionContext with both defaulting to true, but as with StreamId, I'm not sure it's useful enough deserve be a top level property on ConnectionContext.

@SteveSandersonMS
Copy link
Member

@jkotalik Is the change to blazor.server.js intentional? I'm guessing not since no .ts files were changed. If unintentional, could you revert that file? Thanks!

@davidfowl
Copy link
Member

Feels like we're missing a shared interface between the MultiplexConnection and ConnectionContext. There should be a common type for the shared properties and methods.

@jkotalik
Copy link
Contributor Author

Feels like we're missing a shared interface between the MultiplexConnection and ConnectionContext. There should be a common type for the shared properties and methods.

I mentioned that in my comment above. Probably; I need to mess around with the generate code to see what I can do.

@jkotalik
Copy link
Contributor Author

I was chatting with @halter73 yesterday, and he mentioned that we may want to consider making the multiplexed factory internal for now due to potential refactoring in how bedrock would work.

If we were to make a base class for ConnectionContext, it would need to be public no matter what. I don't think it's a big deal, just something to note.

@jkotalik jkotalik removed the request for review from SteveSandersonMS February 19, 2020 21:11
@jkotalik
Copy link
Contributor Author

🆙 📅 based on discussion yesterday.

@jkotalik jkotalik force-pushed the jkotalik/multiplexedFactory branch from d817523 to dae4b3d Compare February 24, 2020 21:48
@halter73
Copy link
Member

We need to figure out how and when we're going to internalize this. Can you create an issue @jkotalik?

@jkotalik
Copy link
Contributor Author

Created the above issue.

@jkotalik jkotalik merged commit d8b6823 into master Feb 25, 2020
@jkotalik jkotalik deleted the jkotalik/multiplexedFactory branch February 25, 2020 01:56
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API proposal: IMultiplexedConnectionListenerFactory and company
8 participants