HttpClient: Deadlock in `SendAsync` when `AllowDuplex` is true under specific conditions

[huntharo]

Description

HttpClient will not send the HEADERS frame for requests that do have Content but do not have EndStream set when SendHeadersAsync is called. It appears the only case that this applies to may be AllowDuplex = true in a custom Content object.

Reproduction Steps

Complete example code:

https://github.com/huntharo/httpclient-duplex-deadlock

Video demonstrating the issue and the fix:

Expected behavior

It seems that SendHeadersAsync should always cause a flush as:

• Causing a flush when headers are written appears to be happening for everything except AllowDuplex = true
• Holding headers back seems like the wrong thing to do, ever, in that it will prevent the remote server from starting the request - delays here are bad
• At the very least, requests that have AllowDuplex = true should also be added to the mustFlush list

Actual behavior

The behavior is changed by subsequent requests sent on the same connection:

• Subsequent AllowDuplex = true requests can cause a flush if there are enough parallel requests sent AND they have enough headers set (the values may need to be unique to avoid deduplication)
  • Under the right conditions this can make it appear that the problem does not exist
• Subsequent requests that set ExpectContinue will cause a flush
  • Setting ExpectContinue will always cause this to work, but results in an additional round trip (confirm?) and is not desirable
• Requests that either have no Content or that have EndStream set (essentially: AllowDuplex = false) will cause a flush
  • Sending a GET request (or POST with ready-at-send Content) will cause the stuck request to send its HEADERS frame
  • The HEADERS frame will be delayed up to the time that the GET request is sent

Failure to use one of the above workarounds will cause the request to timeout without ever sending the HEADERS frame.

Regression?

• Did not test prior versions of dotnet
• There is issue HTTP2: Support bidirectional content #1511
  • This issue says to support bidirectional content over Http2
  • The support is nearly 100% there, this was the only issue I discovered in hundreds of millions of requests that read the response body (to completion) before sending the request body
  • My project was observing periodic 5 second hangs that corresponded to a 5 second interval of a "ping" GET request sent over the same HttpClient

Known Workarounds

See actual behavior, but it appears that ExpectContinue = true is a workaround, as is sending a ton of headers.

Configuration

• dotnet 8.0 / also release/8.0 branch built locally as of 2023-12-20
• Mac OS X Sonoma
• ARM64
• The problem does not appear to be specific to this configuration at all

Other information

Possibly the simplest fix is that the callback in SendHeadersAsync should always return true, causing a flush. The nominal case of non-duplex requests essentially always causes this to return true and it seems only this odd case of duplex requests is causing it to return false.

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

Line 1684 in 2f1fbb0

return s.mustFlush || s.endStream;

However, a more narrow fix is to add in mustFlush: ... || duplex here after moving the bool duplex = line to be above the SendHeadersAsync call:

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

Line 1999 in 2f1fbb0

Http2Stream http2Stream = await SendHeadersAsync(request, cancellationToken, mustFlush: shouldExpectContinue || request.IsExtendedConnectRequest).ConfigureAwait(false);

I would love to make the PR and add tests for this after the approach is approved.

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

Issue Details

---

Description

HttpClient will not send the HEADERS frame for requests that do have Content but do not have EndStream set when SendHeadersAsync is called. It appears the only case that this applies to may be AllowDuplex = true in a custom Content object.

Reproduction Steps

Complete example code:

https://github.com/huntharo/httpclient-duplex-deadlock

Video demonstrating the issue and the fix:

Expected behavior

It seems that SendHeadersAsync should always cause a flush as:

• Causing a flush when headers are written appears to be happening for everything except AllowDuplex = true
• Holding headers back seems like the wrong thing to do, ever, in that it will prevent the remote server from starting the request - delays here are bad
• At the very least, requests that have AllowDuplex = true should also be added to the mustFlush list

Actual behavior

The behavior is changed by subsequent requests sent on the same connection:

• Subsequent AllowDuplex = true requests can cause a flush if there are enough parallel requests sent AND they have enough headers set (the values may need to be unique to avoid deduplication)
  • Under the right conditions this can make it appear that the problem does not exist
• Subsequent requests that set ExpectContinue will cause a flush
  • Setting ExpectContinue will always cause this to work, but results in an additional round trip (confirm?) and is not desirable
• Requests that either have no Content or that have EndStream set (essentially: AllowDuplex = false) will cause a flush
  • Sending a GET request (or POST with ready-at-send Content) will cause the stuck request to send its HEADERS frame
  • The HEADERS frame will be delayed up to the time that the GET request is sent

Failure to use one of the above workarounds will cause the request to timeout without ever sending the HEADERS frame.

Regression?

• Did not test prior versions of dotnet
• There is issue HTTP2: Support bidirectional content #1511
  • This issue says to support bidirectional content over Http2
  • The support is nearly 100% there, this was the only issue I discovered in hundreds of millions of requests that read the response body (to completion) before sending the request body
  • My project was observing periodic 5 second hangs that corresponded to a 5 second interval of a "ping" GET request sent over the same HttpClient

Known Workarounds

See actual behavior, but it appears that ExpectContinue = true is a workaround, as is sending a ton of headers.

Configuration

• dotnet 8.0 / also release/8.0 branch built locally as of 2023-12-20
• Mac OS X Sonoma
• ARM64
• The problem does not appear to be specific to this configuration at all

Other information

Possibly the simplest fix is that the callback in SendHeadersAsync should always return true, causing a flush. The nominal case of non-duplex requests essentially always causes this to return true and it seems only this odd case of duplex requests is causing it to return false.

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

Line 1684 in 2f1fbb0

return s.mustFlush || s.endStream;

However, a more narrow fix is to add in mustFlush: ... || duplex here after moving the bool duplex = line to be above the SendHeadersAsync call:

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

Line 1999 in 2f1fbb0

Http2Stream http2Stream = await SendHeadersAsync(request, cancellationToken, mustFlush: shouldExpectContinue || request.IsExtendedConnectRequest).ConfigureAwait(false);

I would love to make the PR and add tests for this after the approach is approved.

Author:	huntharo
Assignees:	-
Labels:	area-System.Net.Http, untriaged
Milestone:	-

[MihaZupan]

See #30295 (comment)

The gist is that if you want the headers to be sent immediately, even if you don't have any data to send yet using a custom HttpContent, you can call FlushAsync on the output stream to force the headers to be sent.

The same is true for HTTP/1.1.

This issue has been marked needs-author-action and may be missing some important information.

[huntharo]

See #30295 (comment)

The gist is that if you want the headers to be sent immediately, even if you don't have any data to send yet using a custom HttpContent, you can call FlushAsync on the output stream to force the headers to be sent.

The same is true for HTTP/1.1.

Thanks for the quick reply!

From looking at the Functional tests, it appears that the behavior you described is untested as the custom HttpContent classes do not perform this FlushAsync call on their Stream and only a single request is sent on each connection / client in each test.

While the behavior is goofy and unfriendly to developers (it took me ~8 hours to figure out this deadlock), I can confirm the FlushAsync works but I'd really like to get tests in place with custom HttpContent implementations that demonstrate this so it is as least somewhat easier for others to discover how to handle it correctly.

Do you think it would be possible to augment the timeout exception to indicate that unflushed HEADERS were pending when the connection timed out?

[CarnaViire]

it appears that the behavior you described is untested as the custom HttpContent classes do not perform this FlushAsync call on their Stream

The behavior is tested, we just do it directly on the stream for clarity, see e.g.:

runtime/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs

Lines 2440 to 2446 in 623cf77

	// Client should have sent the request headers, and the request stream should now be available
	Stream requestStream = await duplexContent.WaitForStreamAsync();
	// Flush the content stream. Otherwise, the request headers are not guaranteed to be sent.
	await requestStream.FlushAsync();
	(int streamId, _) = await connection.ReadAndParseRequestHeaderAsync(readBody: false);

runtime/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs

Lines 716 to 719 in 623cf77

	Stream requestStream = await requestContent.GetStreamAsync();
	await requestStream.FlushAsync();
	await headersReceived.Task;

Though it is definitely possible to incorporate it inside the content implementation as well, e.g. gRPC does it here.

[huntharo]

it appears that the behavior you described is untested as the custom HttpContent classes do not perform this FlushAsync call on their Stream

The behavior is tested, we just do it directly on the stream for clarity, see e.g.:

runtime/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs

Lines 2440 to 2446 in 623cf77

	// Client should have sent the request headers, and the request stream should now be available
	Stream requestStream = await duplexContent.WaitForStreamAsync();
	// Flush the content stream. Otherwise, the request headers are not guaranteed to be sent.
	await requestStream.FlushAsync();
	(int streamId, _) = await connection.ReadAndParseRequestHeaderAsync(readBody: false);

runtime/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs

Lines 716 to 719 in 623cf77

	Stream requestStream = await requestContent.GetStreamAsync();
	await requestStream.FlushAsync();
	await headersReceived.Task;

Though it is definitely possible to incorporate it inside the content implementation as well, e.g. gRPC does it here.

Ah, good pointers, thank you!

Ok, I think we're good here. This is edge case behavior and there are tests as mentioned.