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

Add method to create ConnectionContext from Socket #34522

Closed
shirhatti opened this issue Jul 20, 2021 · 6 comments · Fixed by #34769
Closed

Add method to create ConnectionContext from Socket #34522

shirhatti opened this issue Jul 20, 2021 · 6 comments · Fixed by #34769
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Done This issue has been fixed feature-kestrel ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in!
Milestone

Comments

@shirhatti
Copy link
Contributor

As part of the scenario described by @kevin-montrose in #34344, we'd like for the sockets transport to create a connection context from an already accepted socket. In Kevin's example, the socket is accepted in another process before being marshalled over an IPC mechanism.

namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets
{
    public class SocketTransportFactory
    {
       public static ConnectionContext CreateConnectionContextFromSocket(Socket socket, SocketTransportOptions options);
    }
}

The consumer of this API will implement a custom IConnectionListenerFactory and will call this new static method in their IConnectionListener.AcceptAsync implementation

@shirhatti shirhatti added area-runtime feature-kestrel api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 20, 2021
@davidfowl
Copy link
Member

I'm not sure SocketTransportOptions makes sense here, we might need a new options type or something like this:

public class SocketTransportFactory
{
       public static ConnectionContext CreateConnectionContextFromSocket(Socket socket, 
                                                                         MemoryPool<byte> memoryPool = null, 
                                                                         PipeOptions inputOptions = null, 
                                                                         PipeOptions outputOptions = null, 
                                                                         waitForData = true);
}

@BrennanConroy BrennanConroy added the Needs: Design This issue requires design work before implementating. label Jul 21, 2021
@halter73
Copy link
Member

halter73 commented Jul 21, 2021

I agree we should probably make a new options type. I think we should maintain the pools internally in a service making this API non static. This is necessary for internal pools like SocketSenderPool and nice-to-have if you just want to use the default internal PinnedBlockMemoryPool. How about this?

namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets
{
    // The implementation will also likely be disposable.
    public interface ISocketConnectionContextFactory
    {
        ConnectionContext CreateConnectionContextFromSocket(Socket socket, SocketConnectionOptions options);
    }

    // sealed? I could see extending this for a custom ISocketConnectionContextFactory.
    public class SocketConnectionOptions
    {
        public PipeOptions InputOptions { get; init; } = new PipeOptions();
        public PipeOptions OutputOptions { get; init; } = new PipeOptions();
        public MemoryPool<byte>? MemoryPool { get; init; } = null;
        
        // A mouthful, but consistent with SocketTransportOptions.
        public bool WaitForDataBeforeAllocatingBuffer { get; init; } = true;
    }

@BrennanConroy BrennanConroy added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 23, 2021
@shirhatti
Copy link
Contributor Author

namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets
{
    // The implementation will also likely be disposable.
    public interface ISocketConnectionContextFactory
    {
        ConnectionContext Create(Socket socket, SocketConnectionOptions options);
    }

    // sealed? I could see extending this for a custom ISocketConnectionContextFactory.
    public sealed class SocketConnectionOptions
    {
        public PipeOptions InputOptions { get; init; } = new PipeOptions();
        public PipeOptions OutputOptions { get; init; } = new PipeOptions();
        
        // A mouthful, but consistent with SocketTransportOptions.
        public bool WaitForDataBeforeAllocatingBuffer { get; init; } = true;
        public bool DelaySocketOperations { get; init; } = false;
    }

@shirhatti shirhatti added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jul 26, 2021
@adityamandaleeka adityamandaleeka added this to the 6.0-rc1 milestone Jul 26, 2021
@halter73 halter73 modified the milestones: 6.0-rc1, 5.0.0-rc1 Jul 26, 2021
@adityamandaleeka adityamandaleeka modified the milestones: 5.0.0-rc1, 6.0-rc1 Jul 26, 2021
@halter73 halter73 removed this from the 6.0-rc1 milestone Jul 26, 2021
@adityamandaleeka adityamandaleeka added this to the 6.0-rc1 milestone Jul 26, 2021
@HaoK HaoK removed the Needs: Design This issue requires design work before implementating. label Jul 26, 2021
@halter73
Copy link
Member

halter73 commented Jul 29, 2021

The PR brought to light some issues with making this just an interface and considering it a service. Basically, we don't want to consume it in the socket transport. We just want to make it easier to build a third-party Socket-based IConnectionListenerFactory without losing out on all the perf work and pooling going on in SocketConnection.

Here's my new proposal:

namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets
{
    public sealed class SocketConnectionContextFactory : IDisposable
    {
        public SocketConnectionContextFactory(SocketConnectionFactoryOptions options, ILogger logger);

        public ConnectionContext Create(Socket socket);
        public void Dispose();
    }

    /// <summary>
    /// Options for creating a <see cref="ConnectionContext"/> from a <see cref="Socket"/>.
    /// </summary>
    public sealed class SocketConnectionFactoryOptions
    {
        /// <summary>
        /// The number of I/O queues used to process requests. Set to 0 to directly schedule I/O to the ThreadPool.
        /// </summary>
        /// <remarks>
        /// Defaults to <see cref="Environment.ProcessorCount" /> rounded down and clamped between 1 and 16.
        /// </remarks>
        public int IOQueueCount { get; set; } = Math.Min(Environment.ProcessorCount, 16);

        /// <summary>
        /// Wait until there is data available to allocate a buffer. Setting this to false can increase throughput at the cost of increased memory usage.
        /// </summary>
        /// <remarks>
        /// Defaults to true.
        /// </remarks>
        public bool WaitForDataBeforeAllocatingBuffer { get; set; } = true;

        /// <summary>
        /// Gets or sets the maximum unconsumed incoming bytes the transport will buffer.
        /// </summary>
        public long? MaxReadBufferSize { get; set; } = 1024 * 1024;

        /// <summary>
        /// Gets or sets the maximum outgoing bytes the transport will buffer before applying write backpressure.
        /// </summary>
        public long? MaxWriteBufferSize { get; set; } = 64 * 1024;

        /// <summary>
        /// Inline application and transport continuations instead of dispatching to the threadpool.
        /// </summary>
        /// <remarks>
        /// This will run application code on the IO thread which is why this is unsafe.
        /// It is recommended to set the DOTNET_SYSTEM_NET_SOCKETS_INLINE_COMPLETIONS environment variable to '1' when using this setting to also inline the completions
        /// at the runtime layer as well.
        /// This setting can make performance worse if there is expensive work that will end up holding onto the IO thread for longer than needed.
        /// Test to make sure this setting helps performance.
        /// </remarks>
        public bool UnsafePreferInlineScheduling { get; set; }
    }

@halter73 halter73 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-approved API was approved in API review, it can be implemented labels Jul 29, 2021
@halter73 halter73 changed the title Add static method to create ConnectionContext from Socket Add method to create ConnectionContext from Socket Aug 4, 2021
@ghost
Copy link

ghost commented Aug 4, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 9, 2021
@halter73
Copy link
Member

halter73 commented Aug 12, 2021

We can probably make the Create(Socket) method thread-safe with something like:

var setting = _settings[Interlocked.Increment(ref _settingsIndex) % _settingsCount];

@HaoK HaoK added ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in! Done This issue has been fixed labels Aug 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 12, 2021
@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-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Done This issue has been fixed feature-kestrel ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants