Skip to content

Don't cache CanReuse value #35575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 24, 2021
Merged

Don't cache CanReuse value #35575

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7d3c36
Don't cache CanReuse value
sebastienros Aug 20, 2021
ae3b3f1
Comment on when to use CanReuse
sebastienros Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ public void Initialize(Http2StreamContext context)
{
base.Initialize(context);

CanReuse = false;
_decrementCalled = false;
_completionState = StreamCompletionFlags.None;
InputRemaining = null;
Expand Down Expand Up @@ -107,7 +106,16 @@ public bool ReceivedEmptyRequestBody
}
}

public bool CanReuse { get; private set; }
// We only want to reuse a stream that was not aborted and has completely finished writing.
// This ensures Http2OutputProducer.ProcessDataWrites is in the correct state to be reused.

// CanReuse must be evaluated on the main frame-processing looping after the stream is removed
// from the connection's active streams collection. This is required because a RST_STREAM
// frame could arrive after the END_STREAM flag is received. Only once the stream is removed
// from the connection's active stream collection can no longer be reset, and is safe to
// evaluate for pooling.

public bool CanReuse => !_connectionAborted && HasResponseCompleted;

protected override void OnReset()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would resetting the value to false in OnReset accomplish the same goal?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line on 44 was effectively doing that before. The issue was that !_connectionAborted && HasResponseCompleted was being evaluated too early on a background thread and a RST_STREAM frame could still be processed and abort the stream before pooling. Now CanReuse is evaluated once in the main frame-processing loop right before it's pooled or not.

{
Expand Down Expand Up @@ -164,9 +172,6 @@ public void CompleteStream(bool errored)
// connection's flow-control window.
_inputFlowControl.Abort();

// We only want to reuse a stream that was not aborted and has completely finished writing.
// This ensures Http2OutputProducer.ProcessDataWrites is in the correct state to be reused.
CanReuse = !_connectionAborted && HasResponseCompleted;
}
finally
{
Expand Down