Added the ability to configure the buffer sizes on the client #35107
Conversation
- When data comes in from the transport or sent to the transport, we use the default pipe buffer size on the client (which is 65K). This change makes it possible to configure both the transport buffer and the application buffer so that it can be configured based on users scenarios. - Raise the default limit fro 65K to 1MB on the client side and raise it from 32K (old pipe limit) to 65K on the server side.
ghost
added
the
BrennanConroy
left a comment
BrennanConroy
left a comment
There was a problem hiding this comment.
I hate to say it, but I'm not too concerned about tests for the client for this specific change. We do have tests that check backpressure for the server side that you could copy if you wanted.
I'm more concerned that no tests failed with the server-side buffer size change.
src/SignalR/common/Http.Connections/src/HttpConnectionDispatcherOptions.cs
Show resolved
Hide resolved
src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnectionOptions.cs
Outdated
Show resolved
Hide resolved
You're concerned we don't have a test that verifies the defaults? |
src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnectionOptions.cs
Outdated
Show resolved
Hide resolved
src/SignalR/common/Http.Connections/src/HttpConnectionDispatcherOptions.cs
Outdated
Show resolved
Hide resolved
src/SignalR/common/Http.Connections/src/HttpConnectionDispatcherOptions.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Halter <[email protected]>
More like, we should have some tests that send a message that's bigger than the limit to make sure it applies backpressure. But maybe we lower the limit in the test so we only send a couple bytes. But a test to check the default would be nice. |
We do that already. |
|
Right, I'm assuming we do lower the limit in the test which is why the change didn't affect anything.
|
Are you worried about the defaults or the feature? |
|
Defaults, pretty sure the feature is well tested |
I think if the feature works, it doesn't matter that we write a similar pause resume tests with the defaults. I don't mind writing a test that asserts what the defaults are though. Thoughts? |
| // We initialize these lazily based on the state of the options specified here. | ||
| // Though these are mutable it's extremely rare that they would be mutated past the | ||
| // call to initialize the routerware. | ||
| internal PipeOptions TransportPipeOptions => _transportPipeOptions ??= new PipeOptions(pauseWriterThreshold: TransportMaxBufferSize, resumeWriterThreshold: TransportMaxBufferSize / 2, readerScheduler: PipeScheduler.ThreadPool, useSynchronizationContext: false); |
There was a problem hiding this comment.
Should we disallow -1 for these sizes? I don't think this is unique to SignalR, but after discussing it in API review, I realize this would lead to weird behavior where you'd get System.IO.Pipeline's default pauseWriterThreshold (64k), but not the default resumeWriterThreshold (0 instead of 32k) meaning all the data would need to be consumed before backpressure is relieved.
There was a problem hiding this comment.
Oh I thought -1 was unlimited, but it's defaults. It might make sense for the API to not allow -1 then.
Fixes #17797
PS: I haven't figured out how to test this as yet.