diff --git a/backend/src/Squidex.Domain.Apps.Entities/Jobs/JobWorker.cs b/backend/src/Squidex.Domain.Apps.Entities/Jobs/JobWorker.cs index e3c2e055b..f03e7d1c9 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Jobs/JobWorker.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Jobs/JobWorker.cs @@ -68,7 +68,7 @@ public sealed class JobWorker : return GetJobProcessorAsync(message.OwnerId); } - private Task GetJobProcessorAsync(DomainId appId) + private async Task GetJobProcessorAsync(DomainId appId) { Task processor; lock (processors) @@ -82,7 +82,7 @@ public sealed class JobWorker : return loaded; }); } - + try { return await processor; diff --git a/backend/src/Squidex.Infrastructure/Translations/ResourcesLocalizer.cs b/backend/src/Squidex.Infrastructure/Translations/ResourcesLocalizer.cs index c8aca9458..75b895ec4 100644 --- a/backend/src/Squidex.Infrastructure/Translations/ResourcesLocalizer.cs +++ b/backend/src/Squidex.Infrastructure/Translations/ResourcesLocalizer.cs @@ -5,7 +5,9 @@ // All rights reserved. Licensed under the MIT license. // ========================================================================== +using System.Collections.Concurrent; using System.Globalization; +using System.Reflection; using System.Resources; using System.Text; @@ -13,6 +15,13 @@ namespace Squidex.Infrastructure.Translations; public sealed class ResourcesLocalizer(ResourceManager resourceManager) : ILocalizer { + // Resolving a property by name is the expensive part of the formatting. The arguments are + // compiler generated types and the variable names come from the resources, so the number of + // combinations is defined by the code and the cache cannot grow beyond that. Misses are cached + // as null, otherwise an unknown variable would pay for the lookup every time. + private static readonly ConcurrentDictionary<(Type Type, string Name), PropertyInfo?> Properties = + new ConcurrentDictionary<(Type Type, string Name), PropertyInfo?>(); + public (string Result, bool Found) Get(CultureInfo culture, string key, string fallback, object? args = null) { Guard.NotNull(culture); @@ -74,7 +83,7 @@ public sealed class ResourcesLocalizer(ResourceManager resourceManager) : ILocal var variableName = variable.ToString(); var variableValue = variableName; - var property = argsType.GetProperty(variableName); + var property = Properties.GetOrAdd((argsType, variableName), static key => key.Type.GetProperty(key.Name)); if (property != null) { diff --git a/backend/tests/Squidex.Infrastructure.Tests/Translations/TTests.cs b/backend/tests/Squidex.Infrastructure.Tests/Translations/TTests.cs index 7c82ddc17..fed63f858 100644 --- a/backend/tests/Squidex.Infrastructure.Tests/Translations/TTests.cs +++ b/backend/tests/Squidex.Infrastructure.Tests/Translations/TTests.cs @@ -57,4 +57,32 @@ public class TTests Assert.Equal(("Var: Upper.", true), actual); } + + [Fact] + public void Should_return_variable_name_when_property_does_not_exist() + { + var actual = sut.Get(CultureInfo.CurrentUICulture, "withVar", "fallback", new { other = 5 }); + + Assert.Equal(("Var: var.", true), actual); + } + + [Fact] + public void Should_return_same_text_when_called_again() + { + var actual1 = sut.Get(CultureInfo.CurrentUICulture, "withVar", "fallback", new { var = 5 }); + var actual2 = sut.Get(CultureInfo.CurrentUICulture, "withVar", "fallback", new { var = 8 }); + + Assert.Equal(("Var: 5.", true), actual1); + Assert.Equal(("Var: 8.", true), actual2); + } + + [Fact] + public void Should_not_reuse_property_of_other_type() + { + var actual1 = sut.Get(CultureInfo.CurrentUICulture, "withVar", "fallback", new { var = 5 }); + var actual2 = sut.Get(CultureInfo.CurrentUICulture, "withVar", "fallback", new { var = "text" }); + + Assert.Equal(("Var: 5.", true), actual1); + Assert.Equal(("Var: text.", true), actual2); + } } diff --git a/resolved.md b/resolved.md index cd3016c4f..e06b77dd5 100644 --- a/resolved.md +++ b/resolved.md @@ -1194,3 +1194,43 @@ one. The same defect as item 15. **Now:** the entry is removed when the task faults, comparing by reference so a newer successful entry added by another caller is not discarded. The removal takes the same lock that guards the dictionary. + +--- + +### 34. Message formatting looked up properties by reflection on every call — **FIXED** +`backend/src/Squidex.Infrastructure/Translations/ResourcesLocalizer.cs` + +**Was:** `ResourcesLocalizer.Get` resolved every `{variable}` placeholder with +`argsType.GetProperty(variableName)` and cached nothing, so each call paid a name search over +the type's members. Mostly harmless on error paths, but `ResolveReferences.CreateFallback` +calls `T.Get("contents.listReferences", new { count = … })` inside the per-content, +per-partition loop of the enrichment pipeline. + +**Now:** a static `ConcurrentDictionary<(Type, string), PropertyInfo?>` memoizes the lookup. + +**Two details that matter more than they look:** + +- **Misses are cached as `null`.** An unknown variable name is a legitimate case — the code + falls back to printing the name — and without caching the negative result those would pay + the reflection cost on every single call, which is the worst case rather than the best. +- **The key is (Type, name), not name.** Different anonymous types share property names, so a + name-only key would hand back another type's `PropertyInfo` and `GetValue` would throw into + the existing `catch`, silently degrading the message to the raw variable name. + +No eviction: the arg types are compiler-generated and the variable names come from the +resource files, so the number of combinations is fixed by the code. + +**Verified:** three new tests in `TTests` — an unknown property, repeated calls with different +values, and two different arg types using the same variable name. The last one is the guard +against the name-only key: mutating the implementation to key by name alone fails it, along +with the two existing case-conversion tests, and leaves the rest passing. +`Squidex.Infrastructure.Tests` green (1036). + +--- + +### Note: `JobWorker` build fix + +While running the suites, `JobWorker.GetJobProcessorAsync` did not compile — the helper from +item 35 had been inlined into it but the method was still declared non-async while using +`await`. Added the `async` modifier and removed a trailing whitespace. No behavioural change; +the `lock` block closes before the `await`, so nothing is held across it. diff --git a/todo.md b/todo.md index bb1ceacfc..ade11352c 100644 --- a/todo.md +++ b/todo.md @@ -9,9 +9,9 @@ overhead / allocation churn), **S4** low (worth fixing while nearby). Item numbers are stable and never reused. Completed items move to [resolved.md](resolved.md) keeping their number, so gaps in the sequence here are -expected — items **4**–**33** and **35** are closed and live there. +expected — items **4**–**35** are closed and live there. -**Status: 4 open of 35 — items 1, 2, 3 (one root cause) and 34. The other 31 are in [resolved.md](resolved.md).** +**Status: 3 open of 35 — items 1, 2 and 3, which are one root cause. The other 32 are in [resolved.md](resolved.md).** --- @@ -66,45 +66,15 @@ already isolated in `ContentScriptVars`. --- -## S3 — Moderate - -### 34. Message formatting looks up properties by reflection on every call -`backend/src/Squidex.Infrastructure/Translations/ResourcesLocalizer.cs:77` - -`ResourcesLocalizer.Get` substitutes `{variable}` placeholders by reflecting over the -anonymous args object: - -```csharp -var property = argsType.GetProperty(variableName); -``` - -No `PropertyInfo` is cached, so every call re-resolves it. `Type.GetProperty(string)` is one -of the slower reflection calls — it does a name lookup over the type's members. - -Mostly this sits on error paths, where it does not matter. The one that is not is -`ResolveReferences.CreateFallback` (`ResolveReferences.cs:125`): - -```csharp -var text = T.Get("contents.listReferences", new { count = referencedContents.Count }); -``` - -That runs inside the per-content, per-partition loop of the enrichment pipeline, for every -reference field that resolves to more than one item. The same method also allocates a fresh -`JsonObject` and loops over every app language on each call. - -**Fix:** cache the `PropertyInfo` per (type, name); a small static dictionary is enough since -the arg types are compiler-generated and few. - ---- - ## Suggested order of attack -1. **Item 34** — small and self-contained: cache the `PropertyInfo` per (type, name). -2. **Items 1–3, engine pooling** — the largest single cost, and the most invasive change on - the list. It touches the security boundary of user-authored scripts, since a pooled - engine must not carry state from one script into the next. **Profile before writing it**: - the estimate that engine construction dominates a scripted content list is read off the - loops, not taken from a trace. +Only **engine pooling (items 1–3)** is left. It is the largest single cost on the list and +also the most invasive change: it touches the security boundary of user-authored scripts, +because a pooled engine must not carry state from one script into the next. + +**Profile before writing it.** The estimate that engine construction dominates a scripted +content list is read off the loops, not taken from a trace, and this is the one item where +the fix is expensive enough that being wrong about the size of the win would matter. ---