Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Useless variable closure #14869

Merged
merged 5 commits into from
Dec 21, 2023
Merged

Useless variable closure #14869

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b9361fb
Fix Media Worker
jtkech Dec 9, 2023
8966f84
another typo
jtkech Dec 9, 2023
540db7c
Separate name for lambda variable
jtkech Dec 19, 2023
69fe184
Merge remote-tracking branch 'origin/main' into jtkech/media-worker
jtkech Dec 20, 2023
e9da33a
Missing change
jtkech Dec 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/OrchardCore.Modules/OrchardCore.Media/Services/MediaFileStoreResolverMiddleware.cs
View file
Open in desktop
Piedone marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public async Task Invoke(HttpContext context)
return;
}

// subpath.Value returns an unescaped path value, subPath returns an escaped path value.
// subPath.Value returns an unescaped path value, subPath returns an escaped path value.
var subPathValue = subPath.Value;

var isFileCached = await _mediaFileStoreCache.IsCachedAsync(subPathValue);
Expand All @@ -79,7 +79,7 @@ public async Task Invoke(HttpContext context)

// When multiple requests occur for the same file we use a Lazy<Task>
// to initialize the file store request once.
await _workers.GetOrAdd(subPathValue, x => new Lazy<Task>(async () =>
await _workers.GetOrAdd(subPathValue, subPathValue => new Lazy<Task>(async () =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How this fixes the issue?!!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to simulate something if I have time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay no problem I will update it

{
try
{
Expand All @@ -94,7 +94,7 @@ public async Task Invoke(HttpContext context)
catch (Exception ex)
{
// Log the error, and pass to pipeline to handle as 404.
// Multiple requests at the same time will all recieve the same 404
// Multiple requests at the same time will all receive the same 404
// as we use LazyThreadSafetyMode.ExecutionAndPublication.
_logger.LogError(ex, "Error retrieving file from media file store for request path {Path}", subPathValue);
}
Expand Down