Split lock to prevent deadlock in Http2Stream.

[ManickaP]

Http2Connection.ChangeInitialWindowSize locks connection's SyncObject and calls Http2Stream.OnWindowUpdate which locks stream's SyncObject.
Http2Stream.Complete is called only while stream's SyncObject lock is take and then it calls Http2Connection.RemoveStream that locks connection SyncObject.

Fixes #48220

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Http2Connection.ChangeInitialWindowSize locks connection's SyncObject and calls Http2Stream.OnWindowUpdate which locks stream's SyncObject.
Http2Stream.Complete is called only while stream's SyncObject lock is take and then it calls Http2Connection.RemoveStream that locks connection SyncObject.

Author:	ManickaP
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[ManickaP]

/azp list

[azure-pipelines]

CI/CD Pipelines for this repository:

• coreclr-gc-longrunning
• coreclr-gc-simulator
• runtime-coreclr outerloop
• runtime-coreclr jitstress
• runtime-coreclr jitstressregs
• runtime-coreclr jitstress2-jitstressregs
• runtime-coreclr r2r-extra
• runtime-coreclr jitstress-isas-x86
• runtime-coreclr jitstress-isas-arm
• runtime-coreclr jitstressregs-x86
• runtime-coreclr libraries-jitstressregs
• runtime-coreclr libraries-jitstress2-jitstressregs
• runtime-coreclr r2r
• runtime-coreclr runincontext
• runtime-coreclr crossgen2
• runtime-libraries-coreclr outerloop
• runtime-libraries-coreclr outerloop-windows
• runtime-libraries-coreclr outerloop-linux
• runtime-libraries-coreclr outerloop-osx
• runtime
• runtime-libraries enterprise-linux
• runtime-libraries stress-http
• runtime-libraries stress-ssl
• runtime-dev-innerloop
• runtime-coreclr crossgen2 outerloop
• coreclr-release-outerloop-nightly
• sync-runtime-to-mono
• runtime-coreclr crossgen2-composite
• runtime-jit-experimental
• runtime-coreclr libraries-jitstress
• dotnet-linker-tests
• runtime-coreclr ilasm
• runtime-coreclr clrinterpreter
• runtime-coreclr crossgen2-composite gcstress
• runtime-libraries-mono outerloop
• runtime-staging

[ManickaP]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[geoffkizer]

All this credit logic has changed since the last time I looked at it, so my understanding may be out of date here. That said...

Is there some reason that the new lock needs to reside on Http2Stream, as opposed to being encapsulated within the credit management logic? The latter seems a lot safer, and that's how it worked originally if I recall correctly.

There are several references to _creditWaiter in SendDataAsync that do not take the new lock. Should they? They certainly look suspicious.

[geoffkizer]

More generally --

At some point we made a change to not use CreditManager here, and try to use CreditWaiter directly. Perhaps we should revisit this?

[ManickaP]

Is there some reason that the new lock needs to reside on Http2Stream

At the moment it seems more suitable since we need to guard _availableCredit as well.

---

There are several references to _creditWaiter in SendDataAsync that do not take the new lock. Should they? They certainly look suspicious.

Could you please point out where? I just wen through all occurrences of _creditWaiter and couldn't find any except for this:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs

Lines 1249 to 1251 in d68a2d4

	// Logically this is part of the else block above, but we can't await while holding the lock.
	Debug.Assert(_creditWaiter != null);
	sendSize = await _creditWaiter.AsValueTask().ConfigureAwait(false);

But that cannot be inside a lock and never was before.

---

At some point we made a change to not use CreditManager here, and try to use CreditWaiter directly. Perhaps we should revisit this?

It happened here: #32624. Maybe @stephentoub has more to say about the reasons for/against.

[stephentoub]

Maybe @stephentoub has more to say about the reasons for/against.

I did it for the reasons outlined in that PR (#32624): it was using a type that was more complicated and provided more features than was needed while also incurring extra allocations.

If we need an object anyway in order to have a separate lock, I have no qualms with us refactoring this logic into another helper type, allocating an instance of it instead of a separate lock object and then using it for the lock as well. We shouldn't just reuse CreditManager, though, as we'll then be back to allocating a new waiter every time the stream yields.

[geoffkizer]

Could you please point out where?

Sorry, I misread your code. Nevermind...

[geoffkizer]

it was using a type that was more complicated and provided more features than was needed while also incurring extra allocations.

And just to be clear -- "provided more features" refers to the fact that CreditManager supports multiple waiters, whereas in the stream credit case we only ever have one waiter, right?

That makes me feel fine about not using CreditManager here, but still using it other places, since the requirements are different. I just to make sure we are not either (a) duplicating code unnecessarily or (b) missing out on using this optimization in other places, but it sounds like that's not the case here.

[stephentoub]

And just to be clear -- "provided more features" refers to the fact that CreditManager supports multiple waiters, whereas in the stream credit case we only ever have one waiter, right?

Correct (and as part of supporting multiple waiters, it doesn't try to reuse any of them).

[GSPP]

The title says "OnWindowsUpdate". I opened this issue because I wanted to know how a running Windows Update could possibly affect a .NET process 😆

[ManickaP]

I'm working on a functional test for the deadlock since I'm able to reproduce the problem. Please don't merge on my behalf.

[ManickaP]

/backport to release/5.0

[github-actions]

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/541208321

[ManickaP]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ManickaP]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).