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

Sockets factories for Connection Abstractions #40044

Closed
scalablecory opened this issue Jul 28, 2020 · 12 comments
Closed

Sockets factories for Connection Abstractions #40044

scalablecory opened this issue Jul 28, 2020 · 12 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Net
Milestone

Comments

@scalablecory
Copy link
Contributor

scalablecory commented Jul 28, 2020

Update: Partially implemented, the remaining parts needs to be revisited. See #40044 (comment) -- @antonfirsov


Implementation was merged for #1793, sans the sockets factories. This tracks the remaining work to implement that issue.

The SocketsConnectionFactory will be trivial to implement: essentially, one must move the implementation over from SocketsHttpConnectionFactory, and make that class inherit from this one.

SocketsListenerFactory can reuse the same SocketsConnection, so 80% of work is already done for that as well.

class SocketsConnectionFactory : ConnectionFactory
{
    // dual-mode IPv6 socket. See Socket(SocketType socketType, ProtocolType protocolType)
    public SocketsConnectionFactory(SocketType socketType, ProtocolType protocolType);

    // See Socket(AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType)
    public SocketsConnectionFactory(AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType);

    // This must be thread-safe!
    public override ValueTask<Connection> ConnectAsync(EndPoint? endPoint, IConnectionProperties? options = null, CancellationToken cancellationToken = default);

    // These exist to provide an easy way to shim the default behavior.
    // Note: Connect must call this to create its socket.
    protected virtual Socket CreateSocket(AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType, EndPoint? endPoint, IConnectionProperties? options);
    protected virtual Stream CreateStream(Socket socket, IConnectionProperties? options);
    protected virtual IDuplexPipe CreatePipe(Socket socket, IConnectionProperties? options);
}

class SocketsListenerFactory : ConnectionListenerFactory
{
    // dual-mode IPv6 socket. See Socket(SocketType socketType, ProtocolType protocolType)
    public SocketsListenerFactory(SocketType socketType, ProtocolType protocolType);

    // See Socket(AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType)
    public SocketsListenerFactory(AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType);

    // This and the listener's Accept must be thread-safe!
    public override ValueTask<ConnectionListener> BindAsync(EndPoint? endPoint, IConnectionProperties? options = null, CancellationToken cancellationToken = default);

    // These exist to provide an easy way for users to override default behavior.
    // Note: Bind and its listener's Accept must call this to create their sockets.
    protected virtual Socket CreateSocket(AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType, EndPoint? endPoint, IConnectionProperties? options);
}
@scalablecory scalablecory added api-approved API was approved in API review, it can be implemented area-System.Net labels Jul 28, 2020
@scalablecory scalablecory added this to the 5.0.0 milestone Jul 28, 2020
@ghost
Copy link

ghost commented Jul 28, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 28, 2020
@scalablecory scalablecory removed the untriaged New issue has not been triaged by the area owner label Jul 28, 2020
@ghost
Copy link

ghost commented Jul 28, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@ghost
Copy link

ghost commented Jul 28, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@AndyPook
Copy link

HI,

The current bits (ie under Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.Internal ) all seem to be very focused on supporting "servers".
I've been working with David Fowlers Project Bedrock, but only to create clients for existing services (ie kafka, nats). These kind of project always need to copy SocketConnection from there (see also Orleans) because they are internal.

This issue seems to be about sockets implementations of the "new" abstractions, which is great!
(assuming they will be public ?)

The question... will these be appropriate, both in usage and packaging, for the development of clients (not just servers)?

Thanks,

@scalablecory
Copy link
Contributor Author

@AndyPook these are public APIs. SocketsConnectionFactory is for client side and SocketsListenerFactory is for server side.

@AndyPook
Copy link

awesome, thanks
really looking forward to this

@karelz
Copy link
Member

karelz commented Jul 30, 2020

@antonfirsov wants to take it on ...
@geoffkizer can you tell him when it is clear we want both factories in 5.0? (follow up of the ongoing design discussions)

@geoffkizer
Copy link
Contributor

Will do.

@antonfirsov
Copy link
Member

antonfirsov commented Aug 4, 2020

@scalablecory what is the right namespace for these?

In the original proposal socket connection stuff has been placed under System.Net.Connections, but SocketsHttpConnectionFactory has landed under System.Net.Http in the PR. Was that a mistake?

@scalablecory
Copy link
Contributor Author

SocketsHttp... Is HTTP specific.

These two should be in System.Net.Connections.

antonfirsov added a commit that referenced this issue Aug 14, 2020
…40506)

Introduces SocketsConnectionFactory from  #40044 without the factory methods for NetworkStream and IDuplexPipe
@antonfirsov
Copy link
Member

This got now partially implemented for 5.0.

Leftovers:

  1. SocketsListenerFactory
  2. Stream and IDuplexPipe factory methods in SocketsConnectionFactory.

During implementation we found uncertainities around the ownership of disposable resources coupled to the IDuplexPipe created by SocketsConnectionFactory but owned by the Connection object. Ownership of the Socket is fine, but if the pipe wraps another disposable resource, then with the current design there is no way to handle it over to the Connection, since IDuplexPipe is not disposable.

We should revisit and probably slightly alter the design of these factory methods before moving on. When figuring out the ownership semantics, we should also think about how GracefulShutdown shall be implemented with user-provided pipes.

@antonfirsov antonfirsov modified the milestones: 5.0.0, 6.0.0 Aug 14, 2020
@karelz karelz modified the milestones: 6.0.0, Future Apr 23, 2021
@stephentoub
Copy link
Member

I'm going to close this. If this library ever comes back, this can be a part of it, but until then, it's not actionable.

@karelz karelz modified the milestones: Future, 7.0.0 Nov 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net
Projects
None yet
Development

No branches or pull requests

7 participants