Skip to content

Add IConnectionFactory implementation to the socket transport #15019

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

Closed
davidfowl opened this issue Oct 15, 2019 · 3 comments
Closed

Add IConnectionFactory implementation to the socket transport #15019

davidfowl opened this issue Oct 15, 2019 · 3 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions 🥌 Bedrock

Comments

@davidfowl
Copy link
Member

davidfowl commented Oct 15, 2019

Today we have all of the pieces in place the server transport but we haven't exposed anything to do client sockets. There's an internal version of the API we want here so we can start with that as the initial API:

public class SocketConnectionFactory : IConnectionFactory, IAsyncDisposable
{
    public SocketConnectionFactory(IOptions<SocketTransportOptions> options, ILoggerFactory loggerFactory);
    public async ValueTask<ConnectionContext> ConnectAsync(EndPoint endpoint, CancellationToken cancellationToken = default);
}

PS: This will fully enable

Some observations/questions:

  • The SocketConnectionFactory is IAsyncDisposable because it owns the memory pool. Do we want all connections made by the same factory sharing the pool? If yes then this type should maybe track outstanding connections before disposing the factory.
  • As part of these refactorings, we should have a way to construct these types without passing Options.Create(OptionsType).
  • Are there any client options that are different from server options? This spike had a shared options base class, and also had client and server options.
@davidfowl davidfowl added 🥌 Bedrock api-suggestion Early API idea and discussion, it is NOT ready for implementation area-servers labels Oct 15, 2019
@analogrelay analogrelay added this to the 5.0.0-preview1 milestone Oct 15, 2019
@davidfowl
Copy link
Member Author

cc @halter73 did you do this?

@halter73
Copy link
Member

halter73 commented Jan 2, 2020

I did start this. We still need to add docs and there are some things that I don't like about it from a client-usage perspective. E.g:

  1. The ctor takes IOptions<SocketTransportOptions> instead of SocketTransportOptions directly and requires an ILoggerFactory. This means you basically have to use the generic host as demonstrated in the http2cat sample: https://github.com/aspnet/AspNetCore/blob/c7937640a4079465d36441724933eae5a9ca0085/src/Servers/Kestrel/samples/http2cat/Program.cs#L18-L26 https://github.com/aspnet/AspNetCore/blob/c7937640a4079465d36441724933eae5a9ca0085/src/Shared/Http2cat/Http2CatIServiceCollectionExtensions.cs#L13-L19
  2. SocketTransportOptions.MemoryPoolFactory is internal as is SlabMemoryPoolFactory.
  3. The SocketConnectionFactory uses the same SocketConnection as the server meaning that it retains some behavior expected by the server that might not be ideal for a client. One example of this is that the SocketConnection will immediately close the connection fully upon receiving a fin making it impossible to use in a half-open write-only state.
  4. It only supports IPEndPoints at the moment.

@analogrelay analogrelay removed this from the 5.0.0-preview1 milestone Mar 11, 2020
@halter73
Copy link
Member

We have static helpers to create IOptions and ILogger factory so adding new constructors isn't critical. The other stuff like possibly exposing a public memory pool factory option, and supporting other EndPoint types can be done later.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 26, 2020
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions 🥌 Bedrock
Projects
None yet
Development

No branches or pull requests

4 participants