-
-
Notifications
You must be signed in to change notification settings - Fork 6
Display pictures in senses #2412
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.
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
base: develop
Are you sure you want to change the base?
Changes from all commits
23a80cb
15214f1
da91533
d89cb63
5e707d3
1fd212f
20b0789
c6d63ce
933415d
056d35b
1db16ea
a87236c
71181ba
4ffd9f9
36b287e
ba73cf3
7f875d5
ced0680
f5e4bb4
071b5ab
85f6042
eca1d1b
2bb8942
d4fb1fa
42b09ee
4240097
cd7380d
f04df64
634fd83
8c09097
f281bac
de5b5ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| using System.Collections.Concurrent; | ||
| using System.Net; | ||
| using Microsoft.Extensions.Options; | ||
| using MiniLcm.Project; | ||
| using Refit; | ||
|
|
@@ -48,19 +50,46 @@ public async Task DeleteResource(Guid fileId) | |
| /// <param name="fileId">media file Id</param> | ||
| /// <returns></returns> | ||
| /// <exception cref="FileNotFoundException"></exception> | ||
| // Coalesces concurrent downloads of the same file into one shared task. Without this, two | ||
| // requests for a not-yet-cached file (e.g. the UI loading a picture more than once at first | ||
| // render) both see no local resource and both call DownloadResource, whose AddLocalResource | ||
| // inserts a LocalResource keyed by the file id — the second insert then fails with | ||
| // "UNIQUE constraint failed: LocalResource.Id". The first caller starts the task; others | ||
| // await the same one. Keyed by file id (a unique Guid), so different files still download in | ||
| // parallel. | ||
| private static readonly ConcurrentDictionary<Guid, Task<LocalResource>> DownloadTasks = new(); | ||
|
|
||
| public async Task<ReadFileResponse> GetFileStream(Guid fileId) | ||
| { | ||
| var localResource = await resourceService.GetLocalResource(fileId); | ||
| if (localResource is null) | ||
| { | ||
| var connectionStatus = await httpClientProvider.ConnectionStatus(); | ||
| if (connectionStatus == ConnectionStatus.Online) | ||
| var downloadTask = DownloadTasks.GetOrAdd(fileId, async _ => | ||
| { | ||
| // Check again if we have it locally in case another thread completed before the GetOrAdd factory ran | ||
| return await resourceService.GetLocalResource(fileId) ?? await resourceService.DownloadResource(fileId, this); | ||
| }); | ||
|
Comment on lines
+67
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🌐 Web query:
💡 Result: No, the valueFactory delegate in ConcurrentDictionary.GetOrAdd is not guaranteed to run only once under concurrent access [1][2][3]. Because the dictionary invokes the valueFactory outside of its internal locks to prevent performance bottlenecks and potential deadlocks (such as those caused by executing unknown or blocking code while holding a lock), it is possible for multiple threads to execute the valueFactory simultaneously for the same key [1][2]. In such scenarios, while only one of the resulting values will ultimately be stored in the dictionary, the factory may be called multiple times [1][4]. If you require the factory to execute exactly once, a common pattern is to store a Lazy instance in the ConcurrentDictionary instead of the value itself, utilizing the built-in thread safety of the Lazy type to ensure the initialization logic runs only once [3][5]. Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the target file around the referenced lines
sed -n '1,220p' backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs
# Find the resource service implementation and related methods
rg -n "GetLocalResource|DownloadResource|DownloadTasks|TryRemove|LocalResource.Id|UNIQUE constraint failed" backend/FwLite -SRepository: sillsdev/languageforge-lexbox Length of output: 11756
🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's a proposed diff to address the issue. Would this work? Is there a simpler way to implement this? diff --git a/backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs b/backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs
index 1566db6b..d95dfb60 100644
--- a/backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs
+++ b/backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs
@@ -64,11 +64,7 @@ public class LcmMediaService(
var localResource = await resourceService.GetLocalResource(fileId);
if (localResource is null)
{
- var downloadTask = DownloadTasks.GetOrAdd(fileId, async _ =>
- {
- // Check again if we have it locally in case another thread completed before the GetOrAdd factory ran
- return await resourceService.GetLocalResource(fileId) ?? await resourceService.DownloadResource(fileId, this);
- });
+ var downloadTask = GetOrStartDownload(fileId);
try
{
localResource = await downloadTask;
@@ -98,6 +94,36 @@ public class LcmMediaService(
return new(File.OpenRead(localResource.LocalPath), Path.GetFileName(localResource.LocalPath));
}
+ // Single-flight download starter. Uses the *value* overload of GetOrAdd, not the factory
+ // overload: ConcurrentDictionary may invoke a factory more than once under contention, and an
+ // async factory with side effects would start a separate DownloadResource on each invocation —
+ // the losing tasks still run and race to AddLocalResource, reintroducing the duplicate-key
+ // insert. Handing GetOrAdd a not-yet-started TaskCompletionSource.Task means only the caller
+ // whose task actually got stored kicks off the download; everyone else awaits that same task.
+ private Task<LocalResource> GetOrStartDownload(Guid fileId)
+ {
+ var tcs = new TaskCompletionSource<LocalResource>(TaskCreationOptions.RunContinuationsAsynchronously);
+ var task = DownloadTasks.GetOrAdd(fileId, tcs.Task);
+ if (task == tcs.Task)
+ _ = RunDownloadAsync(fileId, tcs);
+ return task;
+ }
+
+ private async Task RunDownloadAsync(Guid fileId, TaskCompletionSource<LocalResource> tcs)
+ {
+ try
+ {
+ // Check again if we have it locally in case another thread committed it before we started.
+ var resource = await resourceService.GetLocalResource(fileId)
+ ?? await resourceService.DownloadResource(fileId, this);
+ tcs.SetResult(resource);
+ }
+ catch (Exception e)
+ {
+ tcs.SetException(e);
+ }
+ }
+
// Media files are proxied (lexbox -> FwHeadless). A cold request — e.g. the first one after the
// backend starts, before its code paths are JIT-warmed — can exceed the proxy's timeout and come
// back as 504 (or 502/503) even though the backend keeps working and warms up. Retrying aThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR description images show four states of the Pictures field in the definition/sense editor: a single fried-kale-greens photo displayed inline, an empty state with a green "+ Picture" button, and a two-slide carousel (smiling man / noodle soup) with prev/next arrows and dot indicators — these are illustrative and not directly relevant to this concurrency question.
If you want something a bit less code, private static readonly ConcurrentDictionary<Guid, Lazy<Task<LocalResource>>> DownloadTasks = new();
private Task<LocalResource> GetOrStartDownload(Guid fileId)
{
var lazy = DownloadTasks.GetOrAdd(fileId, _ => new Lazy<Task<LocalResource>>(
() => DownloadAsync(fileId),
LazyThreadSafetyMode.ExecutionAndPublication));
return lazy.Value;
}
private async Task<LocalResource> DownloadAsync(Guid fileId)
{
return await resourceService.GetLocalResource(fileId) ?? await resourceService.DownloadResource(fileId, this);
}This avoids manually wiring up a Either approach solves the double-download problem; the ✏️ Learnings added
|
||
| try | ||
| { | ||
| localResource = await resourceService.DownloadResource(fileId, this); | ||
| localResource = await downloadTask; | ||
| } | ||
| else | ||
| catch | ||
| { | ||
| return new ReadFileResponse(ReadFileResult.Offline); | ||
| // Did the download fail because we're offline? Return an explicit "we're offline" result | ||
| // so that the frontend can display some appropriate UI. If we're online and yet the download | ||
| // failed, then that's a real error that should be rethrown. | ||
| if (await httpClientProvider.ConnectionStatus() == ConnectionStatus.Offline) | ||
| { | ||
| return new ReadFileResponse(ReadFileResult.Offline); | ||
| } | ||
| throw; | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| finally | ||
| { | ||
| // Once the download completes, we need to remove the task from the ConcurrentDictionary | ||
| // so that a new download can be attempted later by a different thread. That way if the local | ||
| // file is ever deleted (say, by being replaced with a different picture) a new download can start. | ||
| DownloadTasks.TryRemove(new KeyValuePair<Guid, Task<LocalResource>>(fileId, downloadTask)); | ||
| } | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
coderabbitai[bot] marked this conversation as resolved.
|
||
| //todo, consider trying to download the file again, maybe the cache was cleared | ||
|
|
@@ -69,16 +98,19 @@ public async Task<ReadFileResponse> GetFileStream(Guid fileId) | |
| return new(File.OpenRead(localResource.LocalPath), Path.GetFileName(localResource.LocalPath)); | ||
| } | ||
|
|
||
| private async Task<(Stream? stream, string? filename)> RequestMediaFile(Guid fileId) | ||
| { | ||
| var mediaClient = await MediaServerClient(); | ||
| var response = await mediaClient.DownloadFile(fileId); | ||
| if (!response.IsSuccessStatusCode) | ||
| { | ||
| throw new Exception($"Failed to download file {fileId}: {response.StatusCode} {response.ReasonPhrase}"); | ||
| } | ||
| return (await response.Content.ReadAsStreamAsync(), response.Content.Headers.ContentDisposition?.FileName?.Replace("\"", "")); | ||
| } | ||
| // Media files are proxied (lexbox -> FwHeadless). A cold request — e.g. the first one after the | ||
| // backend starts, before its code paths are JIT-warmed — can exceed the proxy's timeout and come | ||
| // back as 504 (or 502/503) even though the backend keeps working and warms up. Retrying a | ||
| // transient failure a few times usually hits the now-warm backend and succeeds. A failed fetch | ||
| // never reaches AddLocalResource, so retrying cannot reintroduce the duplicate-key insert. | ||
| private const int MaxDownloadAttempts = 3; | ||
| private static readonly HashSet<HttpStatusCode> TransientDownloadStatusCodes = | ||
| [ | ||
| HttpStatusCode.RequestTimeout, // 408 | ||
| HttpStatusCode.BadGateway, // 502 | ||
| HttpStatusCode.ServiceUnavailable, // 503 | ||
| HttpStatusCode.GatewayTimeout, // 504 | ||
| ]; | ||
|
|
||
| private async Task<IMediaServerClient> MediaServerClient() | ||
| { | ||
|
|
@@ -91,16 +123,32 @@ async Task<DownloadResult> IRemoteResourceService.DownloadResource(string remote | |
| { | ||
| var projectResourceCachePath = ProjectResourceCachePath; | ||
| Directory.CreateDirectory(projectResourceCachePath); | ||
| var (stream, filename) = await RequestMediaFile(new Guid(remoteId)); | ||
| if (stream is null) throw new FileNotFoundException(remoteId); | ||
| await using (stream) | ||
| var mediaClient = await MediaServerClient(); | ||
| var fileId = new Guid(remoteId); | ||
| for (var attempt = 1; ; attempt++) | ||
| { | ||
| filename = Path.GetFileName(filename); | ||
| var localPath = Path.Combine(projectResourceCachePath, filename ?? remoteId); | ||
| localPath = EnsureUnique(localPath); | ||
| await using var localFile = File.Create(localPath); | ||
| await stream.CopyToAsync(localFile); | ||
| return new DownloadResult(localPath); | ||
| using var response = await mediaClient.DownloadFile(fileId); | ||
| if (response.IsSuccessStatusCode) | ||
| { | ||
| await using var stream = await response.Content.ReadAsStreamAsync(); | ||
| if (stream is null) throw new FileNotFoundException(remoteId); | ||
| var filename = response.Content.Headers.ContentDisposition?.FileName?.Replace("\"", ""); | ||
| filename = Path.GetFileName(filename); | ||
| var localPath = Path.Combine(projectResourceCachePath, filename ?? remoteId); | ||
| localPath = EnsureUnique(localPath); | ||
| await using var localFile = File.Create(localPath); | ||
| await stream.CopyToAsync(localFile); | ||
| return new DownloadResult(localPath); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| var statusCode = response.StatusCode; | ||
| var reasonPhrase = response.ReasonPhrase; | ||
| if (attempt >= MaxDownloadAttempts || !TransientDownloadStatusCodes.Contains(statusCode)) | ||
| { | ||
| throw new Exception($"Failed to download file {fileId}: {statusCode} {reasonPhrase}"); | ||
| } | ||
|
|
||
| await Task.Delay(TimeSpan.FromMilliseconds(200)); | ||
| } | ||
| } | ||
|
|
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.