From b84a8d89a3355d49823d5a9a600167d7be296712 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Sharma?= Date: Wed, 8 Nov 2017 09:38:28 -0800 Subject: [PATCH] [Package Signing] Added a Scoped Service Bus Message Handler (#82) Often times, Service Bus message handlers will need to use some sort of Entity Framework context. This requires that the handlers' callbacks are executed within their own dependency injection scope. This change creates a new generic Service Bus message handler `ScopedMessageHandler` that, upon receiving a message, creates a new dependency injection scope. This change also makes `CachingSecretReader` thread-safe, as otherwise, the `ScopedMessageHandler` would need a lock. --- NuGet.Server.Common.sln | 9 ++- .../CachingSecretReader.cs | 58 +++++++++++++----- .../NuGet.Services.ServiceBus.csproj | 1 + .../ScopedMessageHandler.cs | 52 ++++++++++++++++ src/NuGet.Services.ServiceBus/project.json | 1 + .../CachingSecretReaderFacts.cs | 61 ++++--------------- .../NuGet.Services.ServiceBus.Tests.csproj | 1 + .../ScopedMessageHandlerFacts.cs | 47 ++++++++++++++ 8 files changed, 165 insertions(+), 65 deletions(-) create mode 100644 src/NuGet.Services.ServiceBus/ScopedMessageHandler.cs create mode 100644 tests/NuGet.Services.ServiceBus.Tests/ScopedMessageHandlerFacts.cs diff --git a/NuGet.Server.Common.sln b/NuGet.Server.Common.sln index 3c2422a6..9cb1a571 100644 --- a/NuGet.Server.Common.sln +++ b/NuGet.Server.Common.sln @@ -1,7 +1,7 @@  Microsoft Visual Studio Solution File, Format Version 12.00 # Visual Studio 15 -VisualStudioVersion = 15.0.27005.2 +VisualStudioVersion = 15.0.27004.2006 MinimumVisualStudioVersion = 10.0.40219.1 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{8415FED7-1BED-4227-8B4F-BB7C24E041CD}" EndProject @@ -50,6 +50,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "NuGet.Services.Contracts.Te EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "NuGet.Services.ServiceBus.Tests", "tests\NuGet.Services.ServiceBus.Tests\NuGet.Services.ServiceBus.Tests.csproj", "{FF5CA51A-CD6A-463F-AE9A-5737FF0FCFA7}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "NuGet.Services.KeyVault.Tests", "tests\NuGet.Services.KeyVault.Tests\NuGet.Services.KeyVault.Tests.csproj", "{BA1FB5F1-8F6B-4558-862B-F47C3995B06A}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -132,6 +134,10 @@ Global {FF5CA51A-CD6A-463F-AE9A-5737FF0FCFA7}.Debug|Any CPU.Build.0 = Debug|Any CPU {FF5CA51A-CD6A-463F-AE9A-5737FF0FCFA7}.Release|Any CPU.ActiveCfg = Release|Any CPU {FF5CA51A-CD6A-463F-AE9A-5737FF0FCFA7}.Release|Any CPU.Build.0 = Release|Any CPU + {BA1FB5F1-8F6B-4558-862B-F47C3995B06A}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {BA1FB5F1-8F6B-4558-862B-F47C3995B06A}.Debug|Any CPU.Build.0 = Debug|Any CPU + {BA1FB5F1-8F6B-4558-862B-F47C3995B06A}.Release|Any CPU.ActiveCfg = Release|Any CPU + {BA1FB5F1-8F6B-4558-862B-F47C3995B06A}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -156,6 +162,7 @@ Global {E29F54DF-DFB8-4E27-940D-21ECCB9B6FC1} = {7783A106-0F4C-4055-9AB4-413FB2C7B8F0} {79F72C83-E94D-4D04-B904-5A4DA161168E} = {7783A106-0F4C-4055-9AB4-413FB2C7B8F0} {FF5CA51A-CD6A-463F-AE9A-5737FF0FCFA7} = {7783A106-0F4C-4055-9AB4-413FB2C7B8F0} + {BA1FB5F1-8F6B-4558-862B-F47C3995B06A} = {7783A106-0F4C-4055-9AB4-413FB2C7B8F0} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {AA413DB0-5475-4B5D-A3AF-6323DA8D538B} diff --git a/src/NuGet.Services.KeyVault/CachingSecretReader.cs b/src/NuGet.Services.KeyVault/CachingSecretReader.cs index 293ff567..403703bb 100644 --- a/src/NuGet.Services.KeyVault/CachingSecretReader.cs +++ b/src/NuGet.Services.KeyVault/CachingSecretReader.cs @@ -2,7 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; +using System.Collections.Concurrent; using System.Threading.Tasks; namespace NuGet.Services.KeyVault @@ -10,39 +10,65 @@ namespace NuGet.Services.KeyVault public class CachingSecretReader : ISecretReader { public const int DefaultRefreshIntervalSec = 60 * 60 * 24; // 1 day - private readonly int _refreshIntervalSec; private readonly ISecretReader _internalReader; - private readonly Dictionary> _cache; + private readonly ConcurrentDictionary _cache; + private readonly TimeSpan _refreshInterval; public CachingSecretReader(ISecretReader secretReader, int refreshIntervalSec = DefaultRefreshIntervalSec) { - if (secretReader == null) + _internalReader = secretReader ?? throw new ArgumentNullException(nameof(secretReader)); + _cache = new ConcurrentDictionary(); + + _refreshInterval = TimeSpan.FromSeconds(refreshIntervalSec); + } + + public async Task GetSecretAsync(string secretName) + { + if (string.IsNullOrEmpty(secretName)) { - throw new ArgumentNullException(nameof(secretReader)); + throw new ArgumentException("Null or empty secret name", nameof(secretName)); } - _internalReader = secretReader; - _cache = new Dictionary>(); + // If the cache contains the secret and it is not expired, return the cached value. + if (_cache.TryGetValue(secretName, out CachedSecret result) + && !IsSecretOutdated(result)) + { + return result.Value; + } + + // The cache does not contain a fresh copy of the secret. Fetch and cache the secret. + var updatedValue = new CachedSecret(await _internalReader.GetSecretAsync(secretName)); - _refreshIntervalSec = refreshIntervalSec; + return _cache.AddOrUpdate(secretName, updatedValue, (key, old) => updatedValue) + .Value; } - public virtual bool IsSecretOutdated(Tuple cachedSecret) + private bool IsSecretOutdated(CachedSecret secret) { - return DateTime.UtcNow.Subtract(cachedSecret.Item2).TotalSeconds >= _refreshIntervalSec; + return (DateTime.UtcNow - secret.CacheTime) >= _refreshInterval; } - public async Task GetSecretAsync(string secretName) + /// + /// A cached secret. + /// + private class CachedSecret { - if (!_cache.ContainsKey(secretName) || IsSecretOutdated(_cache[secretName])) + public CachedSecret(string value) { - // Get the secret if it is not yet in the cache or it is outdated. - var secretValue = await _internalReader.GetSecretAsync(secretName); - _cache[secretName] = Tuple.Create(secretValue, DateTime.UtcNow); + Value = value; + CacheTime = DateTimeOffset.UtcNow; } - return _cache[secretName].Item1; + /// + /// The value of the cached secret. + /// + public string Value { get; } + + /// + /// The time at which the secret was cached. + /// + public DateTimeOffset CacheTime { get; } } } } \ No newline at end of file diff --git a/src/NuGet.Services.ServiceBus/NuGet.Services.ServiceBus.csproj b/src/NuGet.Services.ServiceBus/NuGet.Services.ServiceBus.csproj index f49cab35..94268b72 100644 --- a/src/NuGet.Services.ServiceBus/NuGet.Services.ServiceBus.csproj +++ b/src/NuGet.Services.ServiceBus/NuGet.Services.ServiceBus.csproj @@ -46,6 +46,7 @@ + diff --git a/src/NuGet.Services.ServiceBus/ScopedMessageHandler.cs b/src/NuGet.Services.ServiceBus/ScopedMessageHandler.cs new file mode 100644 index 00000000..24ab8f64 --- /dev/null +++ b/src/NuGet.Services.ServiceBus/ScopedMessageHandler.cs @@ -0,0 +1,52 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; + +namespace NuGet.Services.ServiceBus +{ + /// + /// Handles messages received by a . + /// Each message will be handled within its own dependency injection scope. + /// + /// The type of messages this handler handles. + public class ScopedMessageHandler + { + /// + /// The factory used to create independent dependency injection scopes for each message. + /// + private readonly IServiceScopeFactory _scopeFactory; + + public ScopedMessageHandler(IServiceScopeFactory scopeFactory) + { + _scopeFactory = scopeFactory ?? throw new ArgumentNullException(nameof(scopeFactory)); + } + + /// + /// Handle the message in its own dependency injection scope. + /// + /// The received message. + /// Whether the message has been handled. If false, the message will be requeued to be handled again later. + public async Task HandleAsync(TMessage message) + { + // Create a new scope for this message. + using (var scope = _scopeFactory.CreateScope()) + { + // Resolve a new message handler for the newly created scope and let it handle the message. + return await ResolveMessageHandler(scope).HandleAsync(message); + } + } + + /// + /// Resolve the message handler given a specific dependency injection scope. + /// + /// The dependency injection scope that should be used to resolve services. + /// The resolved message handler service from the given scope. + private IMessageHandler ResolveMessageHandler(IServiceScope scope) + { + return scope.ServiceProvider.GetRequiredService>(); + } + } +} diff --git a/src/NuGet.Services.ServiceBus/project.json b/src/NuGet.Services.ServiceBus/project.json index 8ec43fa3..1ea393cd 100644 --- a/src/NuGet.Services.ServiceBus/project.json +++ b/src/NuGet.Services.ServiceBus/project.json @@ -1,5 +1,6 @@ { "dependencies": { + "Microsoft.Extensions.DependencyInjection.Abstractions": "1.1.1", "Microsoft.Extensions.Logging": "1.1.2", "Newtonsoft.Json": "9.0.1", "WindowsAzure.ServiceBus": "4.1.3" diff --git a/tests/NuGet.Services.KeyVault.Tests/CachingSecretReaderFacts.cs b/tests/NuGet.Services.KeyVault.Tests/CachingSecretReaderFacts.cs index 2aa8bc01..2be8bce1 100644 --- a/tests/NuGet.Services.KeyVault.Tests/CachingSecretReaderFacts.cs +++ b/tests/NuGet.Services.KeyVault.Tests/CachingSecretReaderFacts.cs @@ -6,6 +6,7 @@ using Moq; using Xunit; using System.Threading; +using System.Diagnostics; namespace NuGet.Services.KeyVault.Tests { @@ -31,27 +32,6 @@ public async Task WhenGetSecretIsCalledCacheIsUsed() Assert.Equal(value1, value2); } - [Theory] - // Secret was refreshed more recently than the refresh interval and is not outdated. - [InlineData(2, 1, false)] - // Secret was refreshed after the refresh interval is outdated - [InlineData(2, 3, true)] - // Secret was refreshed exactly on the refresh interval and is outdated. - [InlineData(2, 2, true)] - public void CorrectlyIdentifiesOutdatedSecrets(int refreshIntervalSec, int secretLastRefreshedSec, bool isOutdated) - { - // Arrange - var cachingSecretReaderMock = new Mock(new Mock().Object, refreshIntervalSec) {CallBase = true}; - var secretToCheck = Tuple.Create("secretName", - DateTime.UtcNow.Add(new TimeSpan(0, 0, -secretLastRefreshedSec))); - - // Act - var result = cachingSecretReaderMock.Object.IsSecretOutdated(secretToCheck); - - // Assert - Assert.Equal(isOutdated, result); - } - [Fact] public async Task WhenGetSecretIsCalledCacheIsRefreshedIfPastInterval() { @@ -62,50 +42,35 @@ public async Task WhenGetSecretIsCalledCacheIsRefreshedIfPastInterval() const int refreshIntervalSec = 1; var mockSecretReader = new Mock(); - mockSecretReader.Setup(x => x.GetSecretAsync(It.IsAny())).Returns(Task.FromResult(firstSecret)); - - var cachingSecretReaderMock = new Mock(mockSecretReader.Object, refreshIntervalSec) - { - CallBase = true - }; - var hasIntervalPassed = false; - cachingSecretReaderMock.Setup(x => x.IsSecretOutdated(It.IsAny>())).Returns(() => - { - // If the interval hasn't passed, the secret we have stored is not outdated. - if (!hasIntervalPassed) - { - return false; - } + mockSecretReader + .SetupSequence(x => x.GetSecretAsync(It.IsAny())) + .Returns(Task.FromResult(firstSecret)) + .Returns(Task.FromResult(secondSecret)); - // If the interval has passed, the secret is outdated. - // It will be refreshed and then the interval will not have passed again. - hasIntervalPassed = false; - return true; - }); + var cachingSecretReader = new CachingSecretReader(mockSecretReader.Object, refreshIntervalSec); // Act - var firstValue1 = await cachingSecretReaderMock.Object.GetSecretAsync(secretName); - var firstValue2 = await cachingSecretReaderMock.Object.GetSecretAsync(secretName); + var firstValue1 = await cachingSecretReader.GetSecretAsync(secretName); + var firstValue2 = await cachingSecretReader.GetSecretAsync(secretName); // Assert mockSecretReader.Verify(x => x.GetSecretAsync(It.IsAny()), Times.Once); Assert.Equal(firstSecret, firstValue1); - Assert.Equal(firstValue1, firstValue2); + Assert.Equal(firstSecret, firstValue2); // Arrange 2 // We are now x seconds later after refreshIntervalSec has passed. - hasIntervalPassed = true; - mockSecretReader.Setup(x => x.GetSecretAsync(It.IsAny())).Returns(Task.FromResult(secondSecret)); + await Task.Delay(TimeSpan.FromSeconds(refreshIntervalSec * 2)); // Act 2 - var secondValue1 = await cachingSecretReaderMock.Object.GetSecretAsync(secretName); - var secondValue2 = await cachingSecretReaderMock.Object.GetSecretAsync(secretName); + var secondValue1 = await cachingSecretReader.GetSecretAsync(secretName); + var secondValue2 = await cachingSecretReader.GetSecretAsync(secretName); // Assert 2 mockSecretReader.Verify(x => x.GetSecretAsync(It.IsAny()), Times.Exactly(2)); Assert.Equal(secondSecret, secondValue1); - Assert.Equal(secondValue1, secondValue2); + Assert.Equal(secondSecret, secondValue2); } } } diff --git a/tests/NuGet.Services.ServiceBus.Tests/NuGet.Services.ServiceBus.Tests.csproj b/tests/NuGet.Services.ServiceBus.Tests/NuGet.Services.ServiceBus.Tests.csproj index 48a7d773..6ffdc036 100644 --- a/tests/NuGet.Services.ServiceBus.Tests/NuGet.Services.ServiceBus.Tests.csproj +++ b/tests/NuGet.Services.ServiceBus.Tests/NuGet.Services.ServiceBus.Tests.csproj @@ -43,6 +43,7 @@ + diff --git a/tests/NuGet.Services.ServiceBus.Tests/ScopedMessageHandlerFacts.cs b/tests/NuGet.Services.ServiceBus.Tests/ScopedMessageHandlerFacts.cs new file mode 100644 index 00000000..9f19fae2 --- /dev/null +++ b/tests/NuGet.Services.ServiceBus.Tests/ScopedMessageHandlerFacts.cs @@ -0,0 +1,47 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; +using Moq; +using Xunit; + +namespace NuGet.Services.ServiceBus.Tests +{ + public class ScopedMessageHandlerFacts + { + [Fact] + public async Task CreatesMessageHandlerUsingScope() + { + // Arrange + var scopeFactoryMock = new Mock(); + var scopeMock = new Mock(); + var serviceProviderMock = new Mock(); + var handlerMock = new Mock>(); + + scopeFactoryMock + .Setup(f => f.CreateScope()) + .Returns(scopeMock.Object); + + scopeMock + .SetupGet(s => s.ServiceProvider) + .Returns(serviceProviderMock.Object); + + serviceProviderMock + .Setup(p => p.GetService(typeof(IMessageHandler))) + .Returns(handlerMock.Object); + + var target = new ScopedMessageHandler(scopeFactoryMock.Object); + + // Act - send two empty messages. + await target.HandleAsync(new object()); + await target.HandleAsync(new object()); + + // Assert + scopeFactoryMock.Verify(f => f.CreateScope(), Times.Exactly(2)); + serviceProviderMock.Verify(p => p.GetService(typeof(IMessageHandler)), Times.Exactly(2)); + handlerMock.Verify(h => h.HandleAsync(It.IsAny()), Times.Exactly(2)); + } + } +}