Useless variable closure

[jtkech]

By using the variable passed to the worker delegate.

[hishamco]

How this fixes the issue?!!

[Piedone]

Wouldn't putting the delegate's body into an actual method be safer?

[jtkech]

Yes and no, still the same problem if used like this

        await _workers.GetOrAdd(
            subPathValue,
            x => new Lazy<Task>(SomeMethodAsync(subPathValue))).Value;

In place of being used like this

        await _workers.GetOrAdd(
            subPathValue,
            x => new Lazy<Task>(SomeMethodAsync(x))).Value;

Or like that

        await _workers.GetOrAdd(
            subPathValue,
            subPathValue => new Lazy<Task>(SomeMethodAsync(subPathValue))).Value;

Would need more time to simulate it because we need concurrent requests but with different media paths, so not so obvious without using a testing app to simulate these requests.

In the meantime, maybe worth to first try the simple suggested change before trying another fix if needed, like using a simple async semaphore.

[Piedone]

Thanks, I'll try, though I couldn't reproduce the issue locally.

[jtkech]

I will try to simulate something if I have time.

[sebastienros]

@jtkech we can update this code (and anywhere we do this gating pattern) with this version which is better: https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/OutputCaching/src/DispatcherExtensions.cs#L8

Usage example: https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/OutputCaching/src/OutputCacheMiddleware.cs#L346

[jtkech]

@sebastienros Thanks for the info, anyway it looks like that #14859 is due to something else.

So this PR doesn't fix anything but maybe I will merge it because at least it prevents an useless variable closure.

[sebastienros]

is due to something else

I know, just mentioning it there. Also I'd rather see the lambda use the x internally instead of renaming the argument, because now it's ambiguous which one is used if you don't know the precedence rule.

[jtkech]

Okay no problem I will update it

[jtkech]

@Piedone

I simulated something with the same pattern and many requested paths, sometimes the same paths sometimes different paths, looks like subPathValue is always well captured and never different than the x parameter passed to the delegate.

Maybe still worth to try this PR to confirm but I don't think it fixes anything.

That said not fully sure and still better to use the passed parameter.

[Piedone]

I'm trying it out now. And while I'm at it, since I'm adding just the changed middleware as an override to an app, I'll also add some custom tracing to it to figure out what actually takes the time.

[jtkech]

Okay cool, let me know