Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Conversation

@DamirAinullin
Copy link
Contributor

Incorrect order of arguments.

@davidfowl
Copy link
Member

davidfowl commented Mar 25, 2018

These are actually correct. The names of the properties are just confusing. They represent what you are writing to:

var transportPipeOptions = new PipeOptions(pauseWriterThreshold: options.TransportMaxBufferSize, resumeWriterThreshold: options.TransportMaxBufferSize / 2, readerScheduler: PipeScheduler.ThreadPool, useSynchronizationContext: false);
var appPipeOptions = new PipeOptions(pauseWriterThreshold: options.ApplicationMaxBufferSize, resumeWriterThreshold: options.ApplicationMaxBufferSize / 2, readerScheduler: PipeScheduler.ThreadPool, useSynchronizationContext: false);
var pair = DuplexPipe.CreateConnectionPair(transportPipeOptions, appPipeOptions);
connection.Transport = pair.Application;
connection.Application = pair.Transport;

and https://github.com/aspnet/KestrelHttpServer/blob/f0629dcbe418839a00ebdbc11fed5c0ae4e32a9b/src/Connections.Abstractions/DuplexPipe.cs#L25

@davidfowl davidfowl closed this Mar 25, 2018
@analogrelay
Copy link
Contributor

The names of the properties are just confusing. They represent what you are writing to

I still go back and forth with these names myself :). I think these are the more sensible names, but I'm not positive :). The idea is that from the Application, you write to/read from the Transport, so the pipe that the application gets is called Transport (and vice-versa). Definitely open to reconsidering these names though.

@davidfowl
Copy link
Member

im open to renaming the argument names and names of the duplex pipe creation but not the propert names on ConnectionContext.

@DamirAinullin
Copy link
Contributor Author

Yes, it is a bit confusing naming. May be it is better to use prefix "from" and "to", for example FromApplicationToTransport or just FromApplication? At least some comment will be good too I think. BTW thanks for such quick reaction for such small problem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants