Fix retry loop on exception #257
Conversation
| } | ||
| } | ||
| } | ||
| public IEnumerable<string> Keys => this.Dictionary.Keys; // TODO Change return type to ICollection<string> in next major release |
There was a problem hiding this comment.
This is wrong and unrelated. Please revert.
There was a problem hiding this comment.
I also noticed the failing test and reverted this change, but using Dictionary.Keys does prevent allocating a new list.
I've updated my branch to fix this, expose it as an internal AllKeys property and added benchmarks:
BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
Intel Core i7-10750H CPU 2.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK=6.0.202
[Host] : .NET 5.0.16 (5.0.1622.16705), X64 RyuJIT
DefaultJob : .NET 5.0.16 (5.0.1622.16705), X64 RyuJIT
| Method | Mean | Error | StdDev | Ratio | RatioSD | Gen 0 | Allocated |
|--------- |----------:|---------:|---------:|------:|--------:|-------:|----------:|
| Keys | 87.77 ns | 1.560 ns | 2.136 ns | 1.00 | 0.00 | 0.0153 | 96 B |
| KeysList | 152.28 ns | 1.699 ns | 1.589 ns | 1.73 | 0.04 | 0.0355 | 224 B |
| AllKeys | 37.82 ns | 0.637 ns | 0.806 ns | 0.43 | 0.02 | 0.0063 | 40 B |
There was a problem hiding this comment.
If you have an improvement it is welcome but it must be part of a separate PR.
|
@ronaldbarendse i appreciate any assistance here but you have to stop bundling random unrelated changes into PRs. If you’ve found an issue please raise one in the tracker and I’ll deal with it. |
| { | ||
| // The image has failed to be returned from the cache. | ||
| // This can happen if the cached image has been physically deleted but the item is still in the LRU cache. | ||
| // We'll retry running the request again in it's entirety. This ensures any changes to the source are tracked also. | ||
| CacheResolverLru.TryRemove(key); | ||
| await this.Invoke(httpContext); | ||
| await this.Invoke(httpContext, false); |
There was a problem hiding this comment.
I'm trying to revisit/rereview this and it's proving impossible with the additional unrelated changes.
The logic here doesn't seem to make sense. You've reversed the condition but are calling the same method (since Invoke calls Invoke(false)).
As I see it the fix should be to pass true as the second parameter.
There was a problem hiding this comment.
I also changed the initial value of retry to true (see above), as I was assuming this boolean indicates whether it should retry the request.
But I now understand this indicates whether it is the retried request (and shouldn't retry again), so the change in #264 is indeed enough to fix it 👍🏻
Prerequisites
Description
PR #240 added a retry in case reading the cached file caused an exception (e.g. when the file got deleted while still in the internal LRU cache). I tested whether that PR fixed the issue, but didn't spot the retry loop: if reading the cached file keeps throwing exceptions, it will go into an infinite loop.
You can test this by getting an exclusive lock on a cached file and requesting that respective URL. You can do this with PowerShell by executing
$fileStream = [System.IO.File]::Open("1f85.jpg", "Open", "Write")(where1f85.jpgis the cached file name). The request will only finish after executing$fileStream.Dispose();to release the exclusive lock...I've also removed allocating a new list to strip unknown commands (AFAIK
Dictionary.Keysuses an existing collection to return the keys) and ensured the middleware first tries to get a valid provider before parsing commands/capturing the HMAC token, removing additional allocations (for invalid requests).