diff --git a/src/Microsoft.Azure.WebJobs.Host/Bindings/Runtime/Binder.cs b/src/Microsoft.Azure.WebJobs.Host/Bindings/Runtime/Binder.cs index 82a6b901f..a069f0ede 100644 --- a/src/Microsoft.Azure.WebJobs.Host/Bindings/Runtime/Binder.cs +++ b/src/Microsoft.Azure.WebJobs.Host/Bindings/Runtime/Binder.cs @@ -19,6 +19,7 @@ namespace Microsoft.Azure.WebJobs /// public class Binder : IBinder, IWatchable, IDisposable { + private readonly object _bindersLock = new object(); private readonly IAttributeBindingSource _bindingSource; private readonly IList _binders = new List(); private readonly RuntimeBindingWatcher _watcher = new RuntimeBindingWatcher(); @@ -54,6 +55,11 @@ internal Binder(IAttributeBindingSource bindingSource) } } + /// + /// For testing only. + /// + internal IList Binders => _binders; + /// /// Gets the binding data. /// @@ -133,7 +139,10 @@ IWatcher IWatchable.Watcher IValueBinder binder = provider as IValueBinder; if (binder != null) { - _binders.Add(binder); + lock (_bindersLock) + { + _binders.Add(binder); + } } IDisposable disposableProvider = provider as IDisposable; @@ -153,7 +162,13 @@ IWatcher IWatchable.Watcher /// internal async Task Complete(CancellationToken cancellationToken) { - foreach (IValueBinder binder in _binders) + IValueBinder[] binders; + lock (_bindersLock) + { + binders = _binders.ToArray(); + } + + foreach (IValueBinder binder in binders) { // Binding can only be uses for non-Out parameters, and their binders ignore this argument. await binder.SetValueAsync(value: null, cancellationToken: cancellationToken); diff --git a/test/Microsoft.Azure.WebJobs.Host.UnitTests/Bindings/Runtime/BinderTests.cs b/test/Microsoft.Azure.WebJobs.Host.UnitTests/Bindings/Runtime/BinderTests.cs new file mode 100644 index 000000000..0e5506265 --- /dev/null +++ b/test/Microsoft.Azure.WebJobs.Host.UnitTests/Bindings/Runtime/BinderTests.cs @@ -0,0 +1,79 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the MIT License. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Azure.WebJobs.Host.Bindings; +using Microsoft.Azure.WebJobs.Host.Bindings.Runtime; +using Microsoft.Azure.WebJobs.Host.Protocols; +using Moq; +using Xunit; + +namespace Microsoft.Azure.WebJobs.Host.UnitTests.Bindings.Runtime; + +public class BinderTests +{ + [Fact] + public async Task Complete_AfterConcurrentBinds_ThrowsNullReferenceException() + { + // Arrange + var mockValueBinder = new Mock(); + mockValueBinder.Setup(m => m.Type).Returns(typeof(object)); + + var mockBinding = new Mock(); + mockBinding.Setup(b => b.BindAsync(It.IsAny())).ReturnsAsync(mockValueBinder.Object); + mockBinding.Setup(b => b.ToParameterDescriptor()).Returns(new ParameterDescriptor()); + + var mockBindingSource = new Mock(); + mockBindingSource.Setup(s => s.BindAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(mockBinding.Object); + + var functionContext = new FunctionBindingContext(Guid.NewGuid(), CancellationToken.None, null); + mockBindingSource.Setup(s => s.AmbientBindingContext).Returns(new AmbientBindingContext(functionContext, new Dictionary().AsReadOnly())); + + var binder = new Binder(mockBindingSource.Object); + var threads = new List(); + + const int numConcurrentThreads = 100; + const int numTasksPerThread = 100; + + // Call BindAsync simultaneously from many threads. There was a race where this would corrupt + // the internal _binders list. + for (int i = 0; i < numConcurrentThreads; i++) + { + threads.Add(new Thread(() => + { + var tasks = new List(); + for (int t = 0; t < numTasksPerThread; t++) + { + tasks.Add(binder.BindAsync(new TestAttribute())); + } + + Task.WaitAll(tasks.ToArray()); + })); + } + + foreach (var thread in threads) + { + thread.Start(); + } + + foreach (var thread in threads) + { + thread.Join(); + } + + // Now that all concurrent binds are done, call Complete. + // If the list's internal state was corrupted by the concurrent Add calls, + // this iteration will likely hit a null element. + await binder.Complete(CancellationToken.None); + + // Because List resizing is not deterministic, another side effect is having a + // different count of binders than expected. + Assert.Equal(numConcurrentThreads * numTasksPerThread, binder.Binders.Count); + } + + private class TestAttribute : Attribute { } +}