From 65e1f1114a8bde460ba8ad88ca7bac0ddaaa6834 Mon Sep 17 00:00:00 2001 From: Marko Lahma Date: Tue, 1 Sep 2026 08:29:42 +0300 Subject: [PATCH] Interop: a host type converter's answer stays with the engine whose converter gave it `TypeResolver`'s accessor cache is shared between the engines using it, and is partitioned by an `InteropResolutionProfile` that reduces a host-installed `ITypeConverter` to a single `bool` - deliberately, because naming the converter itself would pin a host object in a cache that lives for the process. That bool is what keeps a stock engine and a converter engine apart, but it puts *every* engine carrying a converter of its own into one partition. Resolution asks that converter a question. `IndexerAccessor.TryFindIndexer` converts the member *name* to the index type of each single-parameter indexer the type offers, and the one answer decides both whether an `IndexerAccessor` is built and whether the declared member's `PropertyAccessor` is handed an `indexerToTry` to probe ahead of itself. `IsShareable` withheld only the first half, so a type with a `Name` property and a `this[string]` read as `from-property` for a converter that declines the conversion and as `from-indexer` for one that accepts it - and whichever engine reached the type first decided for the other. The mirror case is a key only the indexer can answer, which the narrow engine caches as "no such member" and the wide engine is then served. An engine with a converter of its own now neither answers from the shared cache nor adds to it for a type whose resolution consults that converter, which is a type offering a single-parameter indexer keyed by anything other than `int` - the one index type `TryFindIndexer` settles from the member name alone. The predicate is memoized per resolver and per type, is asked only by an engine whose converter is not the stock one, and is deliberately blind to the member filter: withholding sharing too readily costs such an engine a re-resolution per wrapper, which is what the `IndexerAccessor` exclusion already cost it for the other half of the same decision. Keying the partition on the converter instead was rejected twice over: it would pin the converter and its engine for the life of the process - the reason #3559 gave for keeping an engine's operator resolutions on the engine - and, weakly keyed, it would grow a cache that never evicts by one entry set per engine constructed, which is the retention `ResolvedAccessorCount` exists to bound. Tests are four more facts in Jint.Tests/Runtime/InteropAccessorCacheSharingTests, which already owns this bargain: the two orders of the trace above, the guard that two engines with different converters still share a type no converter is consulted about, and the guard that such engines add nothing to the shared cache per engine, which is what the rejected shape would have broken. Failing first on unfixed 4.x: 2 of 18 on net10.0 and 2 of 18 on net472. Closes #3560 Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S Co-authored-by: Claude Fable 5 --- .../InteropAccessorCacheSharingTests.cs | 115 ++++++++++++++++++ Jint/Runtime/Interop/TypeResolver.cs | 79 +++++++++++- 2 files changed, 192 insertions(+), 2 deletions(-) diff --git a/Jint.Tests/Runtime/InteropAccessorCacheSharingTests.cs b/Jint.Tests/Runtime/InteropAccessorCacheSharingTests.cs index a385890747..e58dc4118b 100644 --- a/Jint.Tests/Runtime/InteropAccessorCacheSharingTests.cs +++ b/Jint.Tests/Runtime/InteropAccessorCacheSharingTests.cs @@ -49,6 +49,23 @@ public sealed class StringIndexed public string this[string key] => key + "!"; } + /// + /// Carries both halves of the decision a host makes during resolution: a + /// declared property an indexer of the same name shadows, and a key only the indexer can answer. + /// + public sealed class Bag + { + private readonly Dictionary _entries = new() + { + ["Name"] = "from-indexer", + ["Extra"] = "indexer-only", + }; + + public string Name => "from-property"; + + public string? this[string key] => _entries.TryGetValue(key, out var value) ? value : null; + } + private sealed class CountingResolver { private int _memberFilterCalls; @@ -296,6 +313,88 @@ public void CustomTypeConvertersPartitionTheCache() custom.Evaluate("host.key").Should().Be("key!"); } + [Fact] + public void TwoCustomTypeConvertersDoNotDecideForEachOther() + { + // Both engines report a host-installed converter, so the profile puts them in the same partition - + // but their converters answer differently, and that answer is what decides whether the declared + // property is handed the indexer to probe ahead of itself (#3560). + var resolver = new TypeResolver(); + var narrow = CreateEngine(resolver, new Bag(), options => options.SetTypeConverter(_ => new NarrowTypeConverter())); + var wide = CreateEngine(resolver, new Bag(), options => options.SetTypeConverter(engine => new WrappingTypeConverter(engine))); + + narrow.Evaluate("host.Name").Should().Be("from-property", "the narrow converter finds no usable indexer"); + narrow.Evaluate("typeof host.Extra").Should().Be("undefined"); + + wide.Evaluate("host.Name").Should().Be("from-indexer", "the wide converter converts the member name to the indexer's key"); + wide.Evaluate("host.Extra").Should().Be("indexer-only"); + } + + [Fact] + public void TwoCustomTypeConvertersDoNotDecideForEachOtherInEitherOrder() + { + var resolver = new TypeResolver(); + var wide = CreateEngine(resolver, new Bag(), options => options.SetTypeConverter(engine => new WrappingTypeConverter(engine))); + var narrow = CreateEngine(resolver, new Bag(), options => options.SetTypeConverter(_ => new NarrowTypeConverter())); + + wide.Evaluate("host.Name").Should().Be("from-indexer"); + narrow.Evaluate("host.Name").Should().Be("from-property"); + + // a fresh wrapper, so the answer is resolved again rather than served from the first one's own store + wide.SetValue("second", new Bag()); + wide.Evaluate("second.Name").Should().Be("from-indexer"); + wide.Evaluate("second.Extra").Should().Be("indexer-only"); + } + + [Fact] + public void CustomTypeConverterEnginesStillShareWhatTheirConverterCannotDecide() + { + // The withholding is per type, not per engine: a type whose members no converter is consulted about + // keeps being resolved once for every engine sharing the resolver. + var counting = new CountingResolver(); + var first = CreateEngine(counting.Resolver, new Host(1), options => options.SetTypeConverter(_ => new NarrowTypeConverter())); + var second = CreateEngine(counting.Resolver, new Host(2), options => options.SetTypeConverter(engine => new WrappingTypeConverter(engine))); + counting.Reset(); + + first.Evaluate("host.Value").Should().Be(1); + counting.Reset().Should().BeGreaterThan(0, "the first engine has to resolve the member"); + + second.Evaluate("host.Value").Should().Be(2); + counting.Reset().Should().Be(0, "Host declares no indexer, so nothing here depends on the converter"); + } + + [Fact] + public void CustomTypeConverterEnginesDoNotGrowTheSharedCachePerEngine() + { + // Keying the partition on the converter itself would be one entry set per engine in a cache that + // never evicts, and would pin every host converter for the life of the process. + var resolver = new TypeResolver(); + + Engine Create() + { + var engine = CreateEngine(resolver, new Bag(), options => options.SetTypeConverter(e => new WrappingTypeConverter(e))); + engine.SetValue("plain", new Host(1)); + return engine; + } + + void Exercise(Engine engine) + { + engine.Evaluate("host.Name").Should().Be("from-indexer"); + engine.Evaluate("plain.Value").Should().Be(1); + } + + Exercise(Create()); + var countAfterFirstEngine = resolver.ResolvedAccessorCount; + countAfterFirstEngine.Should().BeGreaterThan(0, "Host carries no indexer, so its members are still shared"); + + for (var i = 0; i < 20; i++) + { + Exercise(Create()); + } + + resolver.ResolvedAccessorCount.Should().Be(countAfterFirstEngine); + } + #endregion #region 3. nothing engine-affine is shared @@ -325,6 +424,22 @@ public void NestedTypeReferencesStayWithTheirOwnEngine() /// Behaves exactly like the stock converter but is not it, so the engine counts as having a /// host-installed . /// + /// + /// Declines every conversion, including the string to string the stock converter accepts outright, so + /// resolution finds no indexer this member name can be handed to. + /// + private sealed class NarrowTypeConverter : ITypeConverter + { + public object? Convert(object? value, Type type, IFormatProvider formatProvider) + => throw new NotSupportedException(); + + public bool TryConvert(object? value, Type type, IFormatProvider formatProvider, [NotNullWhen(true)] out object? converted) + { + converted = null; + return false; + } + } + private sealed class WrappingTypeConverter : ITypeConverter { private readonly ITypeConverter _inner; diff --git a/Jint/Runtime/Interop/TypeResolver.cs b/Jint/Runtime/Interop/TypeResolver.cs index 0d750ac03f..3443420363 100644 --- a/Jint/Runtime/Interop/TypeResolver.cs +++ b/Jint/Runtime/Interop/TypeResolver.cs @@ -36,6 +36,12 @@ public sealed class TypeResolver /// private readonly ConcurrentDictionary _indexedElementExposure = new(); + /// + /// Memo behind , populated only for engines carrying an + /// of the host's own and therefore having a question to ask at all. + /// + private readonly ConcurrentDictionary _converterKeyedIndexers = new(); + /// /// How many accessors this resolver currently holds. The cache never evicts, so this is the retention /// the resolver commits to: it must stay bounded by the distinct members the engines using it resolve, @@ -87,6 +93,7 @@ private void InvalidateResolvedAccessors() { _reflectionAccessors.Clear(); _indexedElementExposure.Clear(); + _converterKeyedIndexers.Clear(); } /// @@ -304,8 +311,16 @@ internal ReflectionAccessor GetAccessor( var key = new AccessorCacheKey(type, member, requirement, profile); + // Every engine carrying an ITypeConverter of the host's own lands in one partition, because the + // profile can only record that the converter is not the stock one - naming the converter itself + // would pin a host object in a cache that lives for the process. So when resolving this type asks + // that converter a question, the engine neither answers from the shared cache nor adds to it: the + // answer belongs to the converter that gave it, not to the partition every such engine shares + // (#3560). + var converterDecides = !engine._typeConverterIsDefault && ResolutionConsultsConverter(type); + var factories = _reflectionAccessors; - if (factories.TryGetValue(key, out var accessor)) + if (!converterDecides && factories.TryGetValue(key, out var accessor)) { if (throwOnError && ReferenceEquals(accessor, ConstantValueAccessor.NullAccessor) @@ -324,7 +339,7 @@ internal ReflectionAccessor GetAccessor( return accessor; } - if (IsShareable(engine, accessor)) + if (!converterDecides && IsShareable(engine, accessor)) { // racy, we don't care: both racers resolved the same member the same way factories.TryAdd(key, accessor); @@ -360,6 +375,66 @@ private static bool IsShareable(Engine engine, ReflectionAccessor accessor) return true; } + /// + /// Whether resolving a member of asks the engine's + /// anything, and therefore produces an answer that belongs to that converter alone. + /// + /// + /// converts the member name to the index type of + /// every single-parameter indexer the type or one of its interfaces offers; an -keyed + /// one is the only kind settled without the converter, from the name itself. That single answer decides + /// whether the declared member is handed an indexer to probe ahead of itself, so it reaches well past the + /// that withholds. The scan is deliberately + /// blind to the member filter and to which accessors an indexer carries: answering "yes" too readily only + /// costs such an engine a re-resolution per wrapper, while answering "no" wrongly hands one host + /// converter's decision to another. Memoized per resolver and per type, and asked only by an engine whose + /// converter is not the stock one, so the default configuration pays nothing. + /// + private bool ResolutionConsultsConverter( + [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.Interfaces)] Type type) + { + if (_converterKeyedIndexers.TryGetValue(type, out var consults)) + { + return consults; + } + + consults = HasConverterKeyedIndexer(type); + if (!consults) + { + foreach (var interfaceType in type.GetInterfaces()) + { + if (HasConverterKeyedIndexer(interfaceType)) + { + consults = true; + break; + } + } + } + + // GetOrAdd's value overload rather than TryAdd, so a race still hands every caller the same answer + return _converterKeyedIndexers.GetOrAdd(type, consults); + } + + /// + /// Whether offers a single-parameter indexer keyed by anything other than + /// — the one index type keys without + /// consulting the engine's . + /// + private static bool HasConverterKeyedIndexer( + [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] Type type) + { + foreach (var candidate in type.GetProperties()) + { + var indexParameters = candidate.GetIndexParameters(); + if (indexParameters.Length == 1 && indexParameters[0].ParameterType != typeof(int)) + { + return true; + } + } + + return false; + } + private ReflectionAccessor ResolvePropertyDescriptorFactory( Engine engine, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.Interfaces)] Type type,