-
Notifications
You must be signed in to change notification settings - Fork 523
Conversation
{ | ||
Debug.Assert(!_disposedValue, "Block being leased from disposed pool!"); | ||
#else | ||
|
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.
Can you move this after the doc comments or does the compiler complain about the args?
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.
Done
Turns out it doesn fix everything but it does give more info when it goes wrong, just got the following StackTrace
|
Hmm... got really upset with those changes? |
d3c1876
to
cbdb37a
Compare
@davidfowl @CesarBS looks like it can always trigger the issue now... Test is: Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.MaxRequestBufferSizeTests. Alloc stack trace is:
|
cbdb37a
to
415ab1f
Compare
Also fixed 3 tests that weren't handling block properly
7f8e48f
to
83cac5b
Compare
False alarm on the block, that happens after test error - is a symptom not cause. |
@CesarBS Environment.StackTrace causes the MaxRequestBufferSizeTests to bomb; which then causes the block to finalize. Dropping the stack trace. |
83cac5b
to
f42316a
Compare
AppVeyor unrelated #1002 WritesDontCompleteImmediatelyWhenTooManyBytesIncludingNonImmediateAreAlreadyPreCompleted |
Noticed the tests in the PR as no longer slow as molasses. Nice! |
#if DEBUG | ||
public MemoryPoolBlock Lease([CallerMemberName] string memberName = "", | ||
[CallerFilePath] string sourceFilePath = "", | ||
[CallerLineNumber] int sourceLineNumber = 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.
Is it the switching Environment.StackTrace
to this that made the tests faster?
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.
Another one of my super nits :) Put the first arg in its own line, and indent all args with 4 spaces only.
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.
Environment.StackTrace
caused the large response test to crash, which then caused a block to finalize. On the one hand it crashed; on the other it finalized a block - so may still be a circumstance...
Extending #991
In debug:
Also fixed 3 tests that weren't handling blocks properly.
Should resolve #988
Resolves #1012
Resolves #1013
/cc @halter73 @davidfowl @mikeharder