Delay closing SignalR connections on shutdown

[BrennanConroy]

Fixes #25069

namespace Microsoft.AspNetCore.Http.Connections.Features;

public interface IBeforeShutdown
{
    IDisposable Register(Func<Task> callback);
}

[halter73]

Instead of adding a TimeSpan? ShutdownDelay option, can we add a Func<Task> ShutdownCallback option? The implementation could just be a Task.Delay, but it could also be more interesting.

[halter73]

I don't think we should delay at all by default. That slows down the graceful shutdown of the server.

[davidfowl]

I think we should have a default delay, the same way graceful shutdown delays before aggressively closing connections. Think of this similar to the logic we have in Kestrel. We need to give users the chance to gracefully shutdown their streams

[halter73]

I think we should have a default delay, the same way graceful shutdown delays before aggressively closing connections.

The difference is that Kestrel will shutdown immediately if there are no active requests. With this design, even the app developer proactively closes all their connections as soon as ApplicationStopping fires, this will still delay shutdown by an extra second. 😢

[davidfowl]

Here's one thing I think we can do to get the best of both worlds. We need to be the last application stopping event in the pipeline so that applications can block stopping before we do our work. That would allow us to shutdown immediately and have customers code be able to do the sleep/wait/processing

[halter73]

With this design, even the app developer proactively closes all their connections as soon as ApplicationStopping fires, this will still delay shutdown by an extra second. 😢

I just realized I was wrong about this because of _ = CloseConnections(). Why are we even building this list of Tasks then? 😆

We need to be the last application stopping event in the pipeline so that applications can block stopping before we do our work.

@Tratcher had a comment about this when I suggested something similar in triage.

[Tratcher]

We need to be the last application stopping event in the pipeline so that applications can block stopping before we do our work.

How do you guarantee that ordering?

[davidfowl]

I don't think that we can guarantee ordering but I think we should start from that premise and work backwards. If we don't like the timeout idea, when we should think about ways to make it possible to achieve what people want. They want to write code that handles the shutdown before our logic kicks in. Maybe we need a new event...

[halter73]

Does this need to be part of the public API? I was thinking that the IBeforeShutdown implementation cool register the callbacks with an internal service type that we use to trigger the callbacks.

[BrennanConroy]

If it isn't part of the API anyone can come along and replace the implementation and now we have no way of running the callbacks

[halter73]

That could be said for any service with internal dependencies. Anyone who wants the default behavior should be registering with the default service. They can register another service that calls into this if they want.

If the way this API was designed to be used was by replacing the service, we could have just TriggerAsync() on the interface and get rid of Register(Func<Task>)

[halter73]

Why did we decide against something like IDisposable ConnectionOptions.RegisterBeforeShutdown(Func<Task>) again?

[halter73]

I remember. We wanted this to be usable inside of Hub.OnConnectedAsync() and mutating options felt weird there. I still think leaving TriggerAsync() on the public service type is weird though.

[BrennanConroy]

I still think leaving TriggerAsync() on the public service type is weird though.

Agreed, we can decide the interface isn't meant for replacing and remove it

[halter73]

Why is okay fire and forget when we were blocking ApplicationStopping cancellation on these tasks before?

[BrennanConroy]

Closing for now, we're going to be discussing this and can reopen/make a new PR when design is figured out.