0

I'd like to implement an ASP.NET Core middleware (of which only one singleton instance exist) which would maintain an integer request counter, which is incremented every time a request comes in, and is decremented every time a request is finished.

public class GracetermMiddleware
{
    private int requestCount = 0;

    ...

    public async Task Invoke(HttpContext httpContext)
    {
        Interlocked.Increment(ref requestCount);

        try
        {
            await next.Invoke(httpContext);
        }
        finally
        {
            Interlocked.Decrement(ref requestCount);
        }
    }
}

Then what I'd like to do is to have an event handler triggering when the application is stopping, to delay the termination of the process until the outstanding requests are handled. This is how I'm trying to achieve this.

private void OnApplicationStopping()
{
    do
    {
        // Waiting for the outstanding requests to drain
        Thread.Sleep(1000);
    }
    while (requestCount > 0);
}

(I also have some timeout handling which I omitted from the example.)

My question is: do I need to use the volatile keyword when I declare the requestCount field? Can it cause any issues that the field is not volatile?

Mark Vincze
  • 7,737
  • 8
  • 42
  • 81
  • From [the docs](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile): "The volatile modifier is usually used for a field that is accessed by multiple threads without using the lock statement to serialize access." – Camilo Terevinto Sep 14 '18 at 15:20
  • 1
    Why do you want such a middleware? Isn't graceful shutdown already available? – Panagiotis Kanavos Sep 14 '18 at 15:21
  • 1
    @CamiloTerevinto I've read that, but it's still not clear to me if in this specific situation using `volatile` is really necessary (see the docs also saying "usually"). – Mark Vincze Sep 14 '18 at 15:21
  • @PanagiotisKanavos I'm running the API in Kubernetes, which sends a `SIGTERM` to the process at the *beginning* of the shutdown process. Based on our tests, if we don't implement this middleware, then the ASP.NET Core process shutds down abruptly without waiting for the outstanding requests to be finished. – Mark Vincze Sep 14 '18 at 15:23
  • 1
    I think it's safe to take [the docs](https://learn.microsoft.com/en-us/dotnet/api/system.threading.interlocked.increment?view=netcore-2.1) as idiomatic. As such, they indicate that `volatile` isn't required – spender Sep 14 '18 at 15:25
  • @spender you mean in the code sample how the `midpointCount` field is not marked as `volatile`, yet it's being both incremented with `Interlocked.Increment` and read directly at the same time? Nice catch, I haven't seen this, probably this is an example I can follow. Although I'd still be interested in a definitive reasoning about why this is correct. – Mark Vincze Sep 14 '18 at 15:30
  • 1
    @MarkVincze that doesn't mean you need to create a *new* middleware. You need to make the existing feature work. Check [SIGTERM handling](https://github.com/aspnet/Hosting/issues/1527) which discusses this very problem. SIGTERM is handled by the AppDomain.CurrentDomain.ProcessExit event. – Panagiotis Kanavos Sep 14 '18 at 15:35
  • 1
    @MarkVincze this is an XY Problem. The real problem is how to handle SIGTERM and ensure draining works as designed, not how to use volatile. In fact, when similar problems appeared back in 2016 someone created a [GracefulShutdown](https://github.com/avtc/GracefullShutdown) middleware. You could use it as a reference although you *shouldn't* need it any more. The discussion in the `SIGTERM handling` issue shows that you may only need to add a `Thread.Sleep()` in the `AppDomain.CurrentDomain.ProcessExit` event that allows draining to complete – Panagiotis Kanavos Sep 14 '18 at 15:37
  • Thanks for the links @PanagiotisKanavos. But I'd still like to know the correct usage of `volatile` in this case, so let's say that the graceful termination stuff is here just for the sake of having an example. – Mark Vincze Sep 15 '18 at 07:41
  • 1
    Increase the graceful shutdown timeout and see if it helps. – davidfowl Sep 15 '18 at 20:15
  • @davidfowl, yep, in some other APIs I used just a simple `/bin/sleep 30` for the Kubernetes `preStop` hook, and that can already alleviate the problem. But in this question this is just an example, what I'd like to know is what the correct usage (or not using it) of `volatile` would be in this scenario. – Mark Vincze Sep 17 '18 at 08:18
  • Since this question is now boiled down to a *“Do I need to use `volatile` when using the `Interlocked` API?”*, I have closed it with pointers to questions that cover that as part of their answers. – TL;DR: You do not need `volatile` with `Interlocked` and you probably _shouldn’t_ make a field `volatile` if you use it through `Interlocked`. – poke Sep 17 '18 at 10:59
  • I meant the timeout inside the process itself, not kubernetes. – davidfowl Sep 17 '18 at 15:22

0 Answers0