Exit Kestrel heartbeat thread immediately on stop

[JamesNK]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

Noticed in #47580

Kestrel creates a thread and sleeps for a second in its heartbeat:

https://github.com/dotnet/aspnetcore/blob/f295d1f75cd79c6b3abd4f27b8f4b14a58186e7a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/Heartbeat.cs

When Kestrel stops there isn't a way to immediately stop the thread. Thread.Sleep doesn't have cancellation.

We could change it to an AutoResetEvent + OneWait(interval). When Kestrel stops the event can be triggered and exit immediately.

However, one thing to consider is making the heartbeat as reliable as possible. Thread.Sleep vs waiting on and event handle + timeout might have different behavior if the server is under heavy load.

Expected Behavior

The heartbeat thread immediately exits when Kestrel is stopped.

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

No response

Anything else?

No response

[davidfowl]

we could consider switching back to a timer now. The thread pool handles starvation better

cc @stephentoub @kouvel

[danmoseley]

I was going to ask whether Thread.Interrupt() would help here. The sleep behind Thread.Sleep() is alertable, and Interrupt() will break into it.

[stephentoub]

However, one thing to consider is making the heartbeat as reliable as possible. Thread.Sleep vs waiting on an event handle + timeout might have different behavior if the server is under heavy load.

Why would load affect it?

[JamesNK]

A Timer wasn't used here because we didn't want thread pool issues ever to impact the reliability of the heartbeat. In fact, we use the heartbeat event to alert people to thread pool starvation.

I don't know one way or the other about wait handles. Does WaitHandle.WaitOne(TimeSpan.FromSeconds(1)) always promptly return after the timeout?

Perf probably doesn't matter since we're only waiting once a second, but I'm guessing a wait handle is a little less efficient than Thread.Sleep.

[kouvel]

The difference between Sleep and WaitOne would probably be negligible in this case. If Kestrel stopping also implies the process exiting, I'm not sure what would be gained from actively shutting down the heartbeat thread. With #47580 I imagine there could still be race conditions where the heartbeat code could run after disposal.

[JamesNK]

Typically Kestrel is stopped when the process exits, but it can be stopped without killing the process. For example, you could have unit tests that start and stop Kestrel inside each test. A bunch of idle heartbeat threads could build up as tests are running.

Yeah, #47580 is still racey. Kestrel can be stopped anytime, including while the heartbeat is running. The only side effect will likely be a heartbeat subscriber throwing an error back to the heartbeat manager type. The if (!_stopped) { } check in the catch block will suppress logging that.

[JamesNK]

I was going to ask whether Thread.Interrupt() would help here. The sleep behind Thread.Sleep() is alertable, and Interrupt() will break into it.

We have the thread reference. Interrupt would work. However, we'd need to catch the interrupt exception that Thread.Sleep throws and probably need some extra synchronization to prevent interrupts happening after _stopped is checked but before Thread.Sleep.

ManualResetEvent.WaitOne(interval) would have simpler code, but switching to it would be a more significant change to how the heartbeat timer works.