Don't leak AsyncLocal values from synchronous background job methods

This behavior matches the behavior of async methods, where `AsyncLocal` values are allowed to flow into methods (will be useful for context-aware logging in the future), but not allowed to leak them outside of background job methods.
Closes #2409, #906

Author: odinserj

Diff for: src/Hangfire.Core/Server/CoreBackgroundJobPerformer.cs

@@ -154,7 +154,7 @@ private object InvokeMethod(PerformContext context, object instance, object[] ar
         private object InvokeOnTaskScheduler(PerformContext context, BackgroundJobMethod method, Func<object, Task> getTaskFunc)
         {
             var scheduledTask = Task.Factory.StartNew(
-                InvokeSynchronously,
+                InvokeOnTaskSchedulerInternal,
                 method,
                 CancellationToken.None,
                 TaskCreationOptions.None,
@@ -166,6 +166,14 @@ private object InvokeOnTaskScheduler(PerformContext context, BackgroundJobMethod
             return getTaskFunc(result).GetTaskLikeResult(result, method.ReturnType);
         }
 
+        private static object InvokeOnTaskSchedulerInternal(object state)
+        {
+            // ExecutionContext is captured automatically when calling the Task.Factory.StartNew
+            // method, so we don't need to capture it manually. Please see the comment for
+            // synchronous method execution below for details.
+            return ((BackgroundJobMethod)state).Invoke();
+        }
+
         private static object InvokeOnTaskPump(PerformContext context, BackgroundJobMethod method, Func<object, Task> getTaskFunc)
         {
             // Using SynchronizationContext here is the best default option, where workers
@@ -212,9 +220,40 @@ private static object InvokeOnTaskPump(PerformContext context, BackgroundJobMeth
         private static object InvokeSynchronously(object state)
         {
             var method = (BackgroundJobMethod) state;
+            var executionContext = ExecutionContext.Capture();
+
+            if (executionContext != null)
+            {
+                // Asynchronous methods started with the TaskScheduler.StartNew method, capture the
+                // current execution context by default and call the ExecutionContext.Run method on
+                // a thread pool thread to pass the current AsyncLocal values there.
+                //
+                // As a side effect of this call, any updates to AsyncLocal values which happen inside
+                // the background job method itself values aren't passed back to the calling thread
+                // and end their lifetime as soon as method execution is finished.
+                //
+                // Synchronous methods don't need to capture the current execution context because the
+                // thread is not changed. However, any updates to AsyncLocal values that happen inside
+                // the background job method are passed back to the calling thread, expanding their
+                // lifetime to the lifetime of the thread itself. This can result in memory leaks and
+                // possibly affect future background job method executions in unexpected ways.
+                //
+                // To avoid this and to have the same behavior of AsyncLocal between synchronous and
+                // asynchronous methods, we run synchronous ones in a captured execution context. The
+                // ExecutionContext.Run method ensures that AsyncLocal values will be modified only in
+                // the captured context and will not flow back to the parent context.
+                ExecutionContext.Run(executionContext, InvokeSynchronouslyInternal, method);
+                return method.Result;
+            }
+
             return method.Invoke();
         }
 
+        private static void InvokeSynchronouslyInternal(object state)
+        {
+            ((BackgroundJobMethod)state).Invoke();
+        }
+
         private static object[] SubstituteArguments(PerformContext context)
         {
             if (context.BackgroundJob.Job == null)
@@ -240,24 +279,15 @@ private static object[] SubstituteArguments(PerformContext context)
             return result.ToArray();
         }
 
-        private sealed class BackgroundJobMethod
+        private sealed class BackgroundJobMethod(MethodInfo methodInfo, object instance, object[] parameters)
         {
-            private readonly MethodInfo _methodInfo;
-            private readonly object _instance;
-            private readonly object[] _parameters;
-
-            public BackgroundJobMethod(MethodInfo methodInfo, object instance, object[] parameters)
-            {
-                _methodInfo = methodInfo;
-                _instance = instance;
-                _parameters = parameters;
-            }
-
-            public Type ReturnType => _methodInfo.ReturnType;
+            public Type ReturnType => methodInfo.ReturnType;
+            public object Result { get; private set; }
 
             public object Invoke()
             {
-                return _methodInfo.Invoke(_instance, _parameters);
+                Result = methodInfo.Invoke(instance, parameters);
+                return Result;
             }
         }
     }

Diff for: tests/Hangfire.Core.Tests/Server/BackgroundJobPerformerFacts.cs

@@ -1,7 +1,9 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Threading;
+using System.Threading.Tasks;
 using Hangfire.Common;
 using Hangfire.Server;
 using Moq;
@@ -548,9 +550,95 @@ public void Run_ThrowsJobPerformanceException_InsteadOfOperationCanceled_Occurre
             Assert.IsType<OperationCanceledException>(exception.InnerException);
         }
 
-        private BackgroundJobPerformer CreatePerformer()
+#if !NET452
+        [Theory]
+        [MemberData(nameof(GetSchedulers))]
+        public void Run_FlowsAsyncLocal_ThroughFilters_AndSynchronousBackgroundJobMethod(TaskScheduler scheduler)
         {
-            return new BackgroundJobPerformer(_filterProvider.Object, _innerPerformer.Object);
+            // Arrange
+            var id = Guid.NewGuid();
+            _filters.Add(new AsyncLocalFilter(id));
+            _context.BackgroundJob.Job = Job.FromExpression(() => AsyncLocalSync());
+
+            var performer = CreatePerformer(CreateInnerPerformer(scheduler));
+
+            // Act
+            var result = performer.Perform(_context.Object);
+
+            // Assert
+            Assert.Equal(id, result);
+        }
+
+        [Theory]
+        [MemberData(nameof(GetSchedulers))]
+        public void Run_FlowsAsyncLocal_ThroughFilters_AndSimpleAsynchronousBackgroundJobMethod(TaskScheduler scheduler)
+        {
+            // Arrange
+            var id = Guid.NewGuid();
+            _filters.Add(new AsyncLocalFilter(id));
+            _context.BackgroundJob.Job = Job.FromExpression(() => AsyncLocalSimpleAsync());
+
+            var performer = CreatePerformer(CreateInnerPerformer(scheduler));
+
+            // Act
+            var result = performer.Perform(_context.Object);
+
+            // Assert
+            Assert.Equal(id, result);
+        }
+
+        [Theory]
+        [MemberData(nameof(GetSchedulers))]
+        public void Run_FlowsAsyncLocal_ThroughFilters_AndAsyncAwaitAsynchronousBackgroundJobMethod(TaskScheduler scheduler)
+        {
+            // Arrange
+            var id = Guid.NewGuid();
+            _filters.Add(new AsyncLocalFilter(id));
+            _context.BackgroundJob.Job = Job.FromExpression(() => AsyncLocalAsyncAwait());
+
+            var performer = CreatePerformer(CreateInnerPerformer(scheduler));
+
+            // Act
+            var result = performer.Perform(_context.Object);
+
+            // Assert
+            Assert.Equal(id, result);
+        }
+
+        [Theory]
+        [MemberData(nameof(GetSchedulers))]
+        public void Run_FlowsAsyncLocal_ThroughFilters_AndAsyncAwaitContinuationAsynchronousBackgroundJobMethod(TaskScheduler scheduler)
+        {
+            // Arrange
+            var id = Guid.NewGuid();
+            _filters.Add(new AsyncLocalFilter(id));
+            _context.BackgroundJob.Job = Job.FromExpression(() => AsyncLocalAsyncAwaitContinuation());
+
+            var performer = CreatePerformer(CreateInnerPerformer(scheduler));
+
+            // Act
+            var result = performer.Perform(_context.Object);
+
+            // Assert
+            Assert.Equal(id, result);
+        }
+
+        private static CoreBackgroundJobPerformer CreateInnerPerformer(TaskScheduler taskScheduler)
+        {
+            return new CoreBackgroundJobPerformer(new JobActivator(), taskScheduler);
+        }
+
+        public static IEnumerable<object[]> GetSchedulers()
+        {
+            yield return new object[] { null };
+            yield return new object[] { TaskScheduler.Default };
+            yield return new object[] { new Hangfire.Processing.BackgroundTaskScheduler(threadCount: 1) };
+        }
+#endif
+
+        private BackgroundJobPerformer CreatePerformer(IBackgroundJobPerformer inner = null)
+        {
+            return new BackgroundJobPerformer(_filterProvider.Object, inner ?? _innerPerformer.Object);
         }
 
         private Mock<T> CreateFilter<T>()
@@ -561,5 +649,57 @@ private Mock<T> CreateFilter<T>()
 
             return filter;
         }
+
+#if !NET452
+        private static readonly AsyncLocal<Guid> Identifier = new AsyncLocal<Guid>();
+
+        [SuppressMessage("ReSharper", "MemberCanBePrivate.Global")]
+        [SuppressMessage("ReSharper", "UnusedMethodReturnValue.Global")]
+        public static Guid AsyncLocalSync()
+        {
+            return Identifier.Value;
+        }
+
+        [SuppressMessage("ReSharper", "MemberCanBePrivate.Global")]
+        public static Task<Guid> AsyncLocalSimpleAsync()
+        {
+            return Task.FromResult(Identifier.Value);
+        }
+
+        [SuppressMessage("ReSharper", "MemberCanBePrivate.Global")]
+#pragma warning disable CS1998 // Async method lacks 'await' operators and will run synchronously
+        public static async Task<Guid> AsyncLocalAsyncAwait()
+#pragma warning restore CS1998 // Async method lacks 'await' operators and will run synchronously
+        {
+            return Identifier.Value;
+        }
+
+        [SuppressMessage("ReSharper", "MemberCanBePrivate.Global")]
+        public static async Task<Guid> AsyncLocalAsyncAwaitContinuation()
+        {
+            await Task.Yield();
+            return Identifier.Value;
+        }
+
+        private sealed class AsyncLocalFilter : IServerFilter
+        {
+            private readonly Guid _identifier;
+
+            public AsyncLocalFilter(Guid identifier)
+            {
+                _identifier = identifier;
+            }
+
+            public void OnPerforming(PerformingContext context)
+            {
+                Identifier.Value = _identifier;
+            }
+
+            public void OnPerformed(PerformedContext context)
+            {
+                Assert.Equal(_identifier, Identifier.Value);
+            }
+        }
+#endif
     }
 }