Spike an IConnectionFactory for the sockets transport #11672
Conversation
|
|
||
| await socket.ConnectAsync(endPoint); | ||
|
|
||
| var connection = new SocketConnection(socket, memoryPool: null, PipeScheduler.ThreadPool, new SocketsTrace(_logger)); |
There was a problem hiding this comment.
The pool and scheduler might be better as constructor arguments to this factory.
There was a problem hiding this comment.
I say we should get ahead of this and create and options object (though not necessarily ioptions) before we add too many params to the constructor.
davidfowl
added
area-servers
and removed
area-signalr
davidfowl
requested review from
analogrelay,
halter73 and
shirhatti
and removed request for
analogrelay
|
|
||
| namespace System.IO.Pipelines | ||
| { | ||
| public static class PipeReaderExtensions |
There was a problem hiding this comment.
Who's calling these extension methods?
- Made the unix domain sockets test use it
davidfowl
added
the
api-suggestion
davidfowl
force-pushed
the
davidfowl/sockets-client
branch
from
0a02c24 to
e33f2d6
Compare
|
@halter73 give this another look. |
| /// <summary> | ||
| /// The maximum size before the transport will stop proactively reading from the transport. | ||
| /// </summary> | ||
| public long? MaxReadBufferSize { get; set; } = 1024 * 1024; |
There was a problem hiding this comment.
While this is a break from KestrelServerLimits, I think we should standardize on having null be the default value for nullable ints and longs so we can more reasonably change the defaults later. We can use long.MaxValue or -1 to represent unlimited if we feel that's necessary. This should be more consistent with PipeOptions, StreamPipeReaderOptions, and StreamPipeWriterOptions.
There was a problem hiding this comment.
I forgot SocketTransportOptions still derives from this... I guess that means we shouldn't be changing the interpretation of null. It is an inconsistency with PipeOptions, but at leas this is more consistent with other Kestrel-related options I guess 😞
| /// </summary> | ||
| /// <param name="options">The options for this transport</param> | ||
| /// <param name="loggerFactory">The logger factory</param> | ||
| public SocketConnectionFactory(IOptions<SocketClientOptions> options, ILoggerFactory loggerFactory) |
There was a problem hiding this comment.
Do we want a client API to take IOptions? Do we expect this normally to be retrieved from a DI container?
There was a problem hiding this comment.
Tough question but a good one. The only client API we have now is DI enabled.
There was a problem hiding this comment.
You know how we solve this? We leave it DI friendly and then add static factory methods for the non DI case.
var factory = SocketConnectionFactory.Create();
|
This comment was made automatically. If there is a problem contact [email protected]. I've triaged the above build. I've created/commented on the following issue(s) |
|
@anurse I want to get something like this in 3.1. Lets discuss next week on possibilities. |
amcasey
added
area-networking
I'd like to have more than one implementation of the
IConnectionFactoryto make sure the API is sound. This is one potential implementation.