Prevent access to HttpContext throught HttpContextAccessor in TryRefreshAsync #432
Conversation
| @@ -24,9 +25,12 @@ public AzureAppConfigurationRefreshMiddleware(RequestDelegate next, IConfigurati | |||
|
|
|||
| public async Task InvokeAsync(HttpContext context) | |||
| { | |||
| foreach (var refresher in Refreshers) | |||
| using (var flowControl = ExecutionContext.SuppressFlow()) | |||
There was a problem hiding this comment.
I think it needs a comment to explain the reasoning. A suggestion
//
// Configuration refresh is meant to execute as an isolated background task.
// To prevent access of request-based resources, such as HttpContext, we suppress the execution context within the refresh operation.
There was a problem hiding this comment.
Did you get a chance to iterate on this?
| foreach (var refresher in Refreshers) | ||
| // | ||
| // Configuration refresh is meant to execute as an isolated background task. | ||
| // To prevent access of request-based resources, such as HttpContext, we suppress the execution context within the refresh operation. |
There was a problem hiding this comment.
using (var flowControl = ExecutionContext.SuppressFlow())
Can you use an explicit type. It's hard to tell what type flowControl is from the right hand side.
|
Looks good, but need to figure out the destination branch. |
| using (AsyncFlowControl flowControl = ExecutionContext.SuppressFlow()) | ||
| { | ||
| _ = refresher.TryRefreshAsync(); | ||
| foreach (IConfigurationRefresher refresher in Refreshers) | ||
| { | ||
| _ = Task.Run(() => refresher.TryRefreshAsync()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems like an unnecessary amount of work to do on every request. Assume the server is running at 100k RPS. I assume the refreshers themselves are throttled? You still want a check in the middleware to call them at most every 60s(?).
There was a problem hiding this comment.
It is mostly no-op. Refresh only takes action on user-configured expiration interval (default 30s).
| _ = refresher.TryRefreshAsync(); | ||
| foreach (IConfigurationRefresher refresher in Refreshers) | ||
| { | ||
| _ = Task.Run(() => refresher.TryRefreshAsync()); |
There was a problem hiding this comment.
Can this be _ = Task.Run(refresher.TryRefreshAsync);
There was a problem hiding this comment.
Did you mean something else? It cannot convert from 'method group' to 'System.Action'.
| } | ||
| } | ||
|
|
||
| refreshReadyTime = DateTimeOffset.UtcNow.Add(TimeSpan.FromSeconds(1)); |
There was a problem hiding this comment.
Set this before doing the work to prevent other concurrent requests from doing duplicate work. I'd also suggest raising it to 15-60s, config doesn't change very often.
Also, store UtcNow in a variable to avoid calling it twice.
There was a problem hiding this comment.
Thans for the comment, added some logic with help from @jimmyca15 that should help prevent calling the logic in the if statement more than once per second. I don't think we can raise the interval though, since our current minimum refresh interval is 1 second and we wouldn't want to change that.
| private readonly RequestDelegate _next; | ||
| private long _refreshReadyTime = DateTimeOffset.UtcNow.Ticks; | ||
| public IEnumerable<IConfigurationRefresher> Refreshers { get; } |
There was a problem hiding this comment.
Can you add a new-line before Refreshers to separate properties vs fields.
| foreach (var refresher in Refreshers) | ||
| long refreshReadyTime = Interlocked.Read(ref _refreshReadyTime); | ||
|
|
||
| if (refreshReadyTime <= DateTimeOffset.UtcNow.Ticks && |
There was a problem hiding this comment.
DateTimeOffset.UtcNow.Ticks needs a variable, because it should be used in the new value of _refreshReadyTime
There was a problem hiding this comment.
Missed that, thanks
No description provided.