-
Notifications
You must be signed in to change notification settings - Fork 523
Limit size of memory buffer when reading request (#304) #912
Conversation
69016d9
to
fd30294
Compare
} | ||
set | ||
{ | ||
if (value < -1 || value == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use magic values. Use int?
instead.
Can you include the before and after plaintext benchmark results the description? |
// and server, which allow the client to send more than maxInputBufferLength before getting | ||
// paused. We assume the combined buffers are smaller than the difference between | ||
// data.Length and maxInputBufferLength. | ||
Assert.InRange(bytesWritten, maxInputBufferLength.Value - maxSendSize + 1, data.Length - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion is failing on AppVeyor and Travis. It seems that bytesWritten
is usually (but not always) 0. E.g. On AppVeyor:
Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.MaxInputBufferLengthTests.LargeUpload(maxInputBufferLength: 16384, sendContentLengthHeader: True, expectPause: True) [FAIL]
Assert.InRange() Failure
Range: (12289 - 10485759)
Actual: 0
Stack Trace:
MaxInputBufferLengthTests.cs(79,0): at Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.MaxInputBufferLengthTests.<LargeUpload>d__0.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerSer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try a larger timeout, but 100ms is reliable on my dev machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the only issue we're seeing on AppVeyor and Travis is the Block being garbage collected instead of returned to pool
assertion, which @CesarBS is investigating. We'll need to either fix the root cause or disable this assertion for 1.0.0, to unblock the tests for my change.
74570f3
to
f2a4f77
Compare
@@ -321,5 +341,73 @@ private static void ReturnBlocks(MemoryPoolBlock block, MemoryPoolBlock end) | |||
returnBlock.Pool.Return(returnBlock); | |||
} | |||
} | |||
|
|||
private class BufferLengthConnectionController |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think this component should pause and resume. Leave that up to the caller. This couples too many pieces together (especially knowing the changes that are coming). If this is the easiest thing to do for 1.0 then it's fine but this really shouldn't know that much about pausing and resuming the connection and the fact that it has to be posted to the libuv thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This design was also problematic when using a connection filter, since this involves two instances of SocketInput
. Refactoring will be pushed shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we'll have to redo this when the refactoring happens.
e59aabc
to
c8f756c
Compare
|
||
public static IEnumerable<object[]> LargeUploadData | ||
{ | ||
get |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include some rationale for the test data you're creating here i.e. why those values and what you're trying to achieve with each one (or each group).
I'm fine with having the test like this (since we already follow this pattern in other places), but I'm not a fan of this approach. I prefer separate, descriptive tests (e.g. UploadExpectsPause
, BufferLengthControlWorksWithSsl
, etc.) that stand as a spec of what the code is supposed to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a comment for each buffer size explaining why it's worth testing.
I do think the buffer sizes (and whether we expect the client to pause) should be passed as theory data. Creating a new test for each buffer size would be a lot of extra work.
For the 4 combinations of sendContentLengthHeader
and ssl
, it would be reasonable to create 4 tests like:
LargeUploadWithContentLength
LargeUploadWithoutContentLength
LargeUploadWithContentLengthOverSsl
LargeUploadWithoutContentLengthOverSsl
But I still think passing these as boolean parameters is cleaner.
5597da5
to
e928169
Compare
@@ -189,10 +197,16 @@ public MemoryPoolIterator ConsumingStart() | |||
{ | |||
if (!consumed.IsDefault) | |||
{ | |||
var lengthConsumed = new MemoryPoolIterator(_head).GetLength(consumed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only compute this if _bufferLengthControl
is not null
.
c41b27d
to
d32a528
Compare
- Prevents breaking tests which call SocketInput.ctor()
- Add Moq to KestrelTests
Add comments explaining why each size is tested.
- HTTP requires "\r\n", and WriteLine() uses "\n" on Mac/Linux
- Reverts regression introduced in 584f314
… paused until around 10MB after the server sends backpressure.
- Blocking causes deadlock on single-core machines
- Also rename "Length" to "Size" in the implementation classes
…loaded the expected number of bytes. - If test fails, it may now wait forever instead of throwing a nice assert. - However, this change should make the test more robust on slow machines.
4766327
to
ed94ef8
Compare
Creating PR to start getting feedback and run tests on Travis/AppVeyor.
Addresses #304.
Tasks
Connection
,SocketInput
, andKestrelServerOptions
Connection.OnRead()
andIConnectionControl.Pause()
can both call_socket.ReadStop()
and throw.UvStreamHandle.ReadStop()
idempotentUvStreamHandle.ReadStart()
can throw a UvException in some cases (e.g. socket is no longer connected).Socket
toNetworkStream
, so the same test can be reused withSslStream
.Block being garbage collected instead of returned to pool
assertion.Pause()/Resume()
.int?
tolong?
to support buffers larger thanInt32.MaxValue
. Future-proof without breaking change.null
disables the buffer length checking.MaxRequestBufferSize
MaxRequestBufferSize
. Reasonable options:null
: No limit. Least likely to cause issues since execution flow is nearly identical to before this change. However, few customers will change the default, so we are least likely to find issues since the code is not being executed very often, and customers may be surprised if they increase request limit in nginx/IIS but don't changeMaxInputBufferLength
and see Kestrel using a lot of memory.1 MB
: Matches default limit in nginx. Most requests will be under the limit, unless they are large uploads.MaxInputBufferLength - 1
: If the client is sending faster than the server can consume, Pause()/Resume() may be called as often as every packet, which might degrade perf. However, it might also be the most efficient way to do things, since the buffers are kept full and the client is never blocked very long. Another advantage is the client is less likely to get a false-positive timeout.MaxInputBufferLength * 0.5
: Blocks the client until the server has drained at least half the buffer. The client will be paused less often, but each pause will be for a longer time.Connection
.Performance
No observable change in Plaintext or Json RPS.
Notes