[Package Signing] Added a Scoped Service Bus Message Handler #82
Conversation
|
@loic-sharma, |
loic-sharma
changed the title
loic-sharma
requested review from
scottbommarito,
joelverhagen,
agr and
xavierdecoster
|
|
||
| public async Task<string> GetSecretAsync(string secretName) | ||
| { | ||
| // If the cache contains the secret and it is not expired, returned the cached value. |
| } | ||
|
|
||
| public async Task<string> GetSecretAsync(string secretName) | ||
| { |
There was a problem hiding this comment.
Should we have a null-check here?
| /// <summary> | ||
| /// A cached secret. | ||
| /// </summary> | ||
| private struct CachedSecret |
There was a problem hiding this comment.
Not sure whether struct is the right construct here, may be better off using class. This is not a short-lived object, is not immutable, and is likely larger than 16 bytes (DateTime is 8 bytes, string value most likely grows beyond 8 bytes - Unicode is 2 bytes per character).
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/choosing-between-class-and-struct
There was a problem hiding this comment.
Hm. My gut feeling is that since we know the lifetime of the CachedSecret and the CachedSecret is immutable after creation, a struct is better here. I haven't measured performance though, so I'll play it safe and use a class instead.
P.S.: string is implemented as a DWORD and a WCHAR pointer, so a string is only 16 bytes on a 64-bit machine.
There was a problem hiding this comment.
Additionally, it's an anti-pattern to make mutable structs. I agree, class is more appropriate here.
|
|
||
| private readonly ISecretReader _internalReader; | ||
| private readonly Dictionary<string, Tuple<string, DateTime>> _cache; | ||
| private readonly ConcurrentDictionary<string, CachedSecret> _cache; | ||
| private readonly TimeSpan _refreshInterval; | ||
|
|
||
| public CachingSecretReader(ISecretReader secretReader, int refreshIntervalSec = DefaultRefreshIntervalSec) |
There was a problem hiding this comment.
Why doesn't this ctor just accept a TimeSpan?
There was a problem hiding this comment.
Yes, that'd be ideal. Unfortunately, that's a breaking change to a public API, and I don't know the implications of this. I'd like to do the minimal possible work possible to unblock package signing.
There was a problem hiding this comment.
We are introducing a new type which must be adopted. Also, cleaning up our code will never be done unless make improvements along the way. Your call, tho.
| { | ||
| throw new ArgumentNullException(nameof(secretReader)); | ||
| if (!IsSecretOutdated(result)) |
There was a problem hiding this comment.
nit nested if can be collapsed to a single if with an &&.
| /// <summary> | ||
| /// The time at which the secret was cached. | ||
| /// </summary> | ||
| public DateTime CacheTimeUtc { get; set; } |
There was a problem hiding this comment.
Use DateTimeOffset so you don't need to specify UTC in the property name
| public async Task<bool> HandleAsync(TMessage message) | ||
| { | ||
| // Create a new scope for this message. | ||
| using (var scope = _scopeFactory.CreateScope()) |
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
ScopedMessageHandlerthat, upon receiving a message, creates a new dependency injection scope.This change also makes
CachingSecretReaderthread-safe, as otherwise, theScopedMessageHandlerwould need a lock.