Triggering IConnectionLifetimeNotificationFeature on GoAway

[ladeak]

Triggering Connection Lifetime Notification Feature's cancellation on GoAway

On GoAway frames triggering IConnectionLifetimeNotificationFeature's cancellation for gRPC and SignalR to initiate teardown of long running requests.

Description

For Kestrel h2 and h3 triggering IConnectionLifetimeNotificationFeature feature's RequestClose() method on GoAway frames, so that frameworks like SignalR or gRPC may proactively start closing long running request.
h3 control stream (which is the only stream that handles GoAway) also calls StopProcessingNextRequest() method otherwise HttpConnection (subscribed to IConnectionLifetimeNotificationFeature) would send GoAway's from server. h2 already does the same.

Adding new tests: Http3InMemory now implements RequestClose() method and Http2TestBase mimics sharing the same feature collection on ConnectionFeatures and by the Connection's Features properties.

Fixes #42057

[ghost]

Thanks for your PR, @ladeak. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[halter73]

@Tratcher @JamesNK do you think this is useful enough for YARP and/or gRPC to add to .NET 7?

[Tratcher]

I don't think YARP could take advantage of this since it can't (and wouldn't want to) proxy the GoAway to the destination server. YARP would be limited to canceling individual requests/streams which would not be graceful.

[JamesNK]

@Tratcher @JamesNK do you think this is useful enough for YARP and/or gRPC to add to .NET 7?

Is this for the situation where a client has sent a GOAWAY, but the socket is still alive, and it prevents Kestrel from shutting down without hitting a timeout?

I haven't heard anyone bring that up as a gRPC issue.

[ladeak]

@Tratcher @JamesNK do you think this is useful enough for YARP and/or gRPC to add to .NET 7?

I haven't heard anyone bring that up as a gRPC issue.

If you come to the conclusion that maybe then this change should be reconsidered, I was aware that the conversation for the issue still had this open question. Even in this case, I don't mind spending the time on the implementation.

[ladeak]

@halter73 I am a little puzzled: I have prepared a test as suggested with socketsHttpHandler.PooledConnectionIdleTimeout but I don't seem to receive any GoAway frame. I am reviewing Http3Connection, Http3RequestStream and HttpConnectionPool but I am afraid I did not find the place where the GoAway frame would be sent on idle connections. Could you help me point me to the right direction, so that I can better understand this case and prepare the test?

[halter73]

@ladeak Sorry for telling you to go ahead with this and then asking for these tests before more closely looking at the client behavior. I probably shouldn't have even marked it up for grabs until I had done that. I think you're right that SocketsHttpHandler simply never initiates a graceful close of the connection with a GOAWAY frame.

The client is allowed to gracefully close an HTTP/3 connection with a GOAWAY frame according to the HTTP/3 spec, but it certainly isn't required to. The server "SHOULD" send such a frame, but it's not even required to. In the HTTP/2 spec, I think the "SHOULD" goes both for both the server and the client.

@dotnet/ncl Can someone tell me off the top of their head if SocketsHttpHandler ever initiates a graceful GOAWAY for either HTTP/2 or HTTP/3 connections. If so, under what circumstances? Idle timeouts? Disposing the HttpClient with in flight requests?

[MihaZupan]

As far as I know, SocketsHttpHandler will never send a GOAWAY on either HTTP/2 or HTTP/3.
We just close the underlying transport when we're done (disposed && no in flight requests).

[halter73]

Cool. I still don't see the harm in doing this. The spec allows it, and we have the tests where we manually send the GOAWAY frames. I see no reason not to fire a ConnectionClosedRequested in the event a GOAWAY is received. @JamesNK does this make sense to you?

[MihaZupan]

Is it possible/expected that the interface may be notified multiple times for the same connection?

[halter73]

Is it possible/expected that the interface may be notified multiple times for the same connection?

No, but it's backed by a CancellationToken, so we should be good.

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelConnection.cs

Lines 150 to 161 in c7a4ec8

	public void RequestClose()
	{
	try
	{
	_connectionClosingCts.Cancel();
	}
	catch (ObjectDisposedException)
	{
	// There's a race where the token could be disposed
	// swallow the exception and no-op
	}
	}

[JamesNK]

I think the non-test changes are fine.

Is an interop test for HTTP/3 necessary? Because we haven't built a fully-fledged HTTP/3 client for our tests you've needed to hack in parsing H3 frames and QPack in the interop tests. I think the in-memory functional test GOAWAY_TriggersLifetimeNotification_ConnectionClosedRequested is enough.

(we probably should at some point invest in making the in-memory methods support real HTTP/3 over QUIC, but that's a whole other thing)

[ladeak]

I have personally felt much better after having the interop test written - to really see this in action through a socket. (but I also see your point)

@JamesNK and @halter73 , let me know how you decide, and I will remove the test if needed :)

[halter73]

let me know how you decide, and I will remove the test if needed :)

I agree with @JamesNK. I think GOAWAY_TriggersLifetimeNotification_ConnectionClosedRequested is sufficient. I would have liked an HttpClient-based test for this if it were possible, but it's not. Let's remove the interop test.

[JamesNK]

LGTM

[ladeak]

Could someone trigger a new build here?

cc. @halter73

[JamesNK]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[JamesNK]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).