Repository navigation
AOT: serializers resolve their own manifests; guard the TypeCache fallback - #8697
Aaronontheweb merged 5 commits into
Conversation
…lback Closes the last unrooted IL2xxx warning in the AOT canary: Serializer.FromBinary(byte[], string) resolved a wire manifest through Util.Reflection.TypeCache on every call, with no way to turn it off. Supersedes the abandoned akkadotnet#8672, which guarded the fallback but never gave SystemMessageSerializer or the Persistence serializers a table of their own, so with Akka.DynamicTypeLoading off every manifest they saw - including Watch/Unwatch/DeathWatchNotification - started throwing. - SerializerV1Adapter.FromBinary(byte[], string) now just delegates to Inner.FromBinary(bytes, manifest); virtual dispatch already resolves to whatever override Inner has. The old code duplicated the TypeCache lookup itself, silently bypassing a plain Serializer subclass's own string-manifest override. - SystemMessageSerializer, PersistenceMessageSerializer and PersistenceSnapshotSerializer each override FromBinary(byte[], string) with a small static manifest-name -> Type table covering every type their FromBinary(byte[], Type) already handles. PersistentFSM.PersistentFSMSnapshot<> is generic and stays on the base fallback. - Serializer.FromBinary(byte[], string) reorders: a TypeCache hit wins first (free, no switch read); a miss with the switch off throws a clear SerializationException naming the manifest and serializer; only then does it fall to reflection, moved into a [RequiresUnreferencedCode] helper. TypeCache gains a non-reflecting TryGetCached and [RequiresUnreferencedCode] on GetType/ GetTypeInternal. Deletes the TypeCache line from aot-warnings.baseline.txt - the unrooted publish is now warning-free from src/core/Akka (was 1, the TypeCache entry). Tests round-trip every SystemMessage and Persistence envelope type (plus a Snapshot) through the real Serialization.Deserialize with the switch off, a versioned-AQN manifest spelling, the V1 adapter delegating to a serializer's own string override, and the switch-off/on/cached-hit paths on the base fallback.
Review on akkadotnet#8697 found the three manifest tables ran TrySplitTypeName (regex plus substrings) on every FromBinary(byte[], string) call - 465 ns / 176 B versus 56 ns / 24 B beforehand, on a path that includes remote DeathWatch and Persistence recovery by default. - New Akka.Util.TypeExtensions.ManifestTable/TryResolveManifestType: each table is now a Dictionary<string, Type> keyed by both Type.TypeQualifiedName() (what Manifest(obj) writes today) and the bare Type.FullName, built from a plain Type[] - no hand-typed manifest strings. A direct hit is one dictionary lookup; only a miss strips assembly identity and retries once, for a versioned or legacy spelling. - SystemMessageSerializer, PersistenceMessageSerializer and PersistenceSnapshotSerializer switch to the shared helper; PersistenceMessageSerializer's ad hoc TryPersistenceTypeName is gone. - Trimmed the adapter/Serializer.cs remarks and the baseline comment; reverted a whitespace-only change in TypeCache.cs. Tests: - Folded the Remote switch-off round trip into RemoteSerializersSpec (already has DynamicTypeLoadingCollection and switch plumbing) instead of a third copy; it now includes two old-style AssemblyQualifiedName spellings (one with ProcessorArchitecture) so it can't pass on a merely-warm TypeCache. - Added an end-to-end regression test to RemoteDeathWatchSpec: two ActorSystems over remoting, switch off, Watch a remote actor, stop it, expect Terminated - the actual scenario akkadotnet#8672 broke, not just a unit-level manifest lookup. - Removed the redundant switch-on-reflection test from SerializerManifestFallbackSpec; the cache-hit-first test already exercises that path while priming the cache. PR body updated: the hot-path fix, that TypeCache.GetType's [RequiresUnreferencedCode] now gives IL2026 to a consumer's own trim analysis (Breaking changes), and the revised test list.
…thWatch test clears TypeCache so a warm cache can't hide a missing table
Merging dev brought in a second definition of the same class in the same namespace.
Aaronontheweb
left a comment
Aaronontheweb
left a comment
There was a problem hiding this comment.
Author walkthrough: each comment below explains why that change is needed.
| if (string.IsNullOrEmpty(manifest)) | ||
| return FromBinary(bytes, (Type)null); | ||
|
|
||
| if (TypeCache.TryGetCached(manifest, out var cached)) |
There was a problem hiding this comment.
Cache first, so the hot path costs the same as before. The old code called TypeCache.GetType, which is a GetOrAdd, so a hit was already one dictionary lookup. That stays true here. The feature-switch check below only runs on a miss, which happens once per manifest on the JIT, since the miss path caches what it resolves.
| if (TypeCache.TryGetCached(manifest, out var cached)) | ||
| return FromBinary(bytes, cached); | ||
|
|
||
| if (!AkkaFeatures.IsDynamicTypeLoadingSupported) |
There was a problem hiding this comment.
This is the line that removes the AOT warning. IsDynamicTypeLoadingSupported carries [FeatureSwitchDefinition] and [FeatureGuard(typeof(RequiresUnreferencedCodeAttribute))]. With Trim="true" the trimmer turns it into false, deletes the call below, and TypeCache.GetType drops out of the app.
The message names both fixes: resolve your own manifests, or register the type through SerializationSetup. When the switch is on (every JIT app by default), nothing changes.
| return FromBinaryReflecting(bytes, manifest); | ||
| } | ||
|
|
||
| [RequiresUnreferencedCode("Resolves manifest to a Type by reflection through TypeCache. The trimmer cannot tell which type that is, so it may have been trimmed away.")] |
There was a problem hiding this comment.
I split the reflection into its own private method so I could mark it [RequiresUnreferencedCode]. The analyzer accepts the call at line 131 because the feature guard above it covers it. Any future caller without a guard gets a warning instead of a silent trim hazard. The body is the old code, moved here unchanged, with the same exception message.
| } | ||
|
|
||
| return Inner.FromBinary(bytes, type); | ||
| return Inner.FromBinary(bytes, manifest); |
There was a problem hiding this comment.
Serialization.AdaptSerializer wraps every V1 serializer in this adapter. The removed block was a copy of Serializer.FromBinary(byte[], string), and that copy went straight to TypeCache.GetType. So it skipped any FromBinary(byte[], string) override on the inner serializer, including the new manifest tables in this PR, and it skipped the AOT guard.
This is what broke remote DeathWatch in #8672. Virtual dispatch covers every removed branch:
SerializerWithStringManifestoverrides this method, so the call reaches it.- The empty-manifest check and the reflection fallback, with the same error text, now live in the base class.
SerializerManifestFallbackSpec pins this.
| /// If the type is in Akka.dll or in Mscorlib.dll, it is sufficient to supply the type name qualified by its namespace. | ||
| /// </param> | ||
| /// <returns>TBD</returns> | ||
| [RequiresUnreferencedCode("Resolves typeName by reflection when it is not already cached. The trimmer cannot tell which type that is, so it may have been trimmed away. Guard with TryGetCached plus AkkaFeatures.IsDynamicTypeLoadingSupported instead of calling this directly on a cache miss.")] |
There was a problem hiding this comment.
This attribute states something that is already true. It shows up in the API approval file because it's a public member.
- JIT callers: no change, since the attribute only matters to the trim analyzer.
- Trimmed or AOT apps that call
TypeCache.GetTypedirectly: they now get IL2026 at their own call site. That's correct, because the call can fail once types are trimmed.
The only possible break is a trimmed app that builds with warnings as errors. I'll list it in the breaking-changes batch.
| # The generic name->Type cache behind every remaining HOCON type lookup in core. This one only goes | ||
| # away when nothing calls it on a startup path. | ||
| IL2057|src/core/Akka/Util/Reflection/TypeCache.cs|Akka.Util.Reflection.TypeCache.GetTypeInternal(String)|Unrecognizedvaluepassedtotheparameter'typeName'ofmethod'System.Type.GetType(String,Boolean)'.It'snotpossibletoguaranteetheavailabilityofthetargettype. | ||
| # |
There was a problem hiding this comment.
The canary job checks that this entry is gone. Its unrooted publish now reports no Akka warnings, so the baseline keeps only its header and the rooted-only section. On dev, link-validator needs NoWarn IL2104 for this one warning. With this PR it doesn't.
| var hadSwitch = AppContext.TryGetSwitch(switchName, out var previous); | ||
| AppContext.SetSwitch(switchName, false); | ||
| // an earlier test may have cached these types with the switch on, which would hide a missing table | ||
| TypeCache.Clear(); |
There was a problem hiding this comment.
An end-to-end test, because the bug in #8672 only appeared there. The unit tests passed, but remote DeathWatch broke. I confirmed the test can fail: with SystemMessageSerializer's override removed and RemotingSpec run first to warm the cache, it fails. Without this Clear() it would pass on the warm cache.
| Sys.Serialization.Deserialize(bytes, serializer.Identifier, manifest).Should().BeOfType(message.GetType(), message.GetType().Name); | ||
| } | ||
|
|
||
| Sys.Serialization.Deserialize(serializer.ToBinary(new Terminate()), serializer.Identifier, |
There was a problem hiding this comment.
These two manifests use older spellings: a 1.3 Version= and one with ProcessorArchitecture=MSIL. Neither is in TypeCache or matches a table key exactly, so they exercise the strip-and-retry path in TryResolveManifestType. That's the mixed-version cluster case.
| { | ||
| } | ||
|
|
||
| [Fact(DisplayName = "SerializerV1Adapter.FromBinary(bytes, manifest) should call the inner serializer's own override When it has one")] |
There was a problem hiding this comment.
This pins the adapter fix. The inner serializer is a plain Serializer with its own FromBinary(byte[], string), and the manifest names no real type. The old adapter skipped the override and threw from TypeCache. The new one returns the override's result.
| }); | ||
| } | ||
|
|
||
| [Fact(DisplayName = "Serialization should reject an unregistered PersistentFSMSnapshot manifest When dynamic type loading is off")] |
There was a problem hiding this comment.
This pins what happens to the one type left out of the table. With the switch off it must fail with the clear error, not with an obscure reflection exception and not by silently decoding the wrong thing.
The previous update overclaimed in both directions. Fix it against the actual source, each site checked directly: - Classic DotNetty remoting does not work under Native AOT, full stop: EndpointManager, FailureDetectorRegistry and TransportAdapters each resolve a HOCON type name with an unguarded Type.GetType, so startup throws TypeLoadException on the transport class. Artery already ships on dev as an experimental opt-in (akka.remote.artery.enabled, default off), not a future target - no CI canary covers it yet, though. - Akka.Cluster, Akka.Persistence and the cluster tools: their own serializers resolve from built-in tables now, but downing-provider-class, the Persistence journal/snapshot plugin class, event adapters, the DData durable store (ReplicatorSettings) and a custom sharding state store (CustomStateStoreModeProvider) still go through unguarded Type.GetType. Call these unsupported, not "not yet verified". - Split out remote deployment's real site, DaemonMsgCreateSerializer. PropsFromProto, from Props.TypeName/PropsSurrogate, which is a different path (what a serializer carries a Props value through). - Added the stream-refs-with-the-switch-off case (SerializationTools. TypeFromString, akkadotnet#8667) and the uncached-manifest case on the base Serializer.FromBinary(byte[], string) (akkadotnet#8697), including Persistence's PersistentFSM.PersistentFSMSnapshot<> staying on that fallback. - Setup list was wrong: three types take an instance/factory today (LoggerSetup, SerializationSetup, ExtensionsSetup), with BootstrapSetup as the optional fourth - not "four" as peers. - Named the real akka.extensions table entries (DistributedDataProvider, DistributedPubSubExtensionProvider, ClusterClientReceptionistExtensionProvider, ClusterMetricsExtensionProvider) instead of the HOCON short names. - Dropped the trimmed-publish claim CI doesn't check, described what the canaries actually gate on (a Warning/Error log at startup) instead of "nothing warns", and added a short tip on reading per-call-site warnings.
* Docs: update Native AOT support scope for v1.6 native-aot.md predates several AOT milestones merged since: LoggerSetup and ExtensionsSetup now exist, module serializer tables cover Remote, Cluster, the cluster-tools family, Persistence and Streams, and the Akka.Hosting.AOT.App canary now runs in CI alongside the core one. Reword the Remote/Cluster/Persistence note from "unsupported" to "not yet verified" - no canary boots an ActorSystem through any of them under Native AOT yet, which is a different claim than classic DotNetty remoting being broken. Call out Props-based remote deployment as its own still-reflective path, and point to source-generated MessagePack serialization as the trim-safe alternative to the default JSON serializer for user messages. * Docs: correct Native AOT support scope per review The previous update overclaimed in both directions. Fix it against the actual source, each site checked directly: - Classic DotNetty remoting does not work under Native AOT, full stop: EndpointManager, FailureDetectorRegistry and TransportAdapters each resolve a HOCON type name with an unguarded Type.GetType, so startup throws TypeLoadException on the transport class. Artery already ships on dev as an experimental opt-in (akka.remote.artery.enabled, default off), not a future target - no CI canary covers it yet, though. - Akka.Cluster, Akka.Persistence and the cluster tools: their own serializers resolve from built-in tables now, but downing-provider-class, the Persistence journal/snapshot plugin class, event adapters, the DData durable store (ReplicatorSettings) and a custom sharding state store (CustomStateStoreModeProvider) still go through unguarded Type.GetType. Call these unsupported, not "not yet verified". - Split out remote deployment's real site, DaemonMsgCreateSerializer. PropsFromProto, from Props.TypeName/PropsSurrogate, which is a different path (what a serializer carries a Props value through). - Added the stream-refs-with-the-switch-off case (SerializationTools. TypeFromString, #8667) and the uncached-manifest case on the base Serializer.FromBinary(byte[], string) (#8697), including Persistence's PersistentFSM.PersistentFSMSnapshot<> staying on that fallback. - Setup list was wrong: three types take an instance/factory today (LoggerSetup, SerializationSetup, ExtensionsSetup), with BootstrapSetup as the optional fourth - not "four" as peers. - Named the real akka.extensions table entries (DistributedDataProvider, DistributedPubSubExtensionProvider, ClusterClientReceptionistExtensionProvider, ClusterMetricsExtensionProvider) instead of the HOCON short names. - Dropped the trimmed-publish claim CI doesn't check, described what the canaries actually gate on (a Warning/Error log at startup) instead of "nothing warns", and added a short tip on reading per-call-site warnings.
1 participant
Why
The last unrooted
IL2xxxwarning in the AOT canary (seesrc/aot/Akka.AOT.App/README.md) camefrom
Serializer.FromBinary(byte[], string), the base-class manifest fallback: it always resolvedthe wire manifest through
Util.Reflection.TypeCache, with no way to turn that off.This supersedes the abandoned #8672. That PR guarded the fallback behind
Akka.DynamicTypeLoadingbut never gave
SystemMessageSerializeror the two Persistence serializers a manifest table oftheir own. Since none of the four first-party serializers that reach this path override
FromBinary(byte[], string), turning the switch off made every manifest they see throw instead ofonly an unrecognized one - including
Watch,UnwatchandDeathWatchNotification, i.e. remoteDeathWatch. Review on that PR caught it and moved it to draft. This PR gives the three in-scope
serializers their own tables first, then guards only the true fallback, after a cache check.
ProtobufSerializer(remote id 2) is intentionally left out - it stays on the guarded reflectionpath and is tracked separately in #8696.
The adapter fix
SerializerV1Adapter.FromBinary(byte[], string)used to duplicate theTypeCachelookup insteadof calling
Inner.FromBinary(bytes, manifest). That meant a plainSerializersubclass (not aSerializerWithStringManifest) that overrides the string-manifest overload itself was silentlybypassed - the adapter went straight to
TypeCacheand ignored the override. It now just delegates:Virtual dispatch resolves this to whatever
Inneractually is, so behavior for every existinginner serializer is unchanged - the base
Serializer.FromBinary(byte[], string)fallback does thesame
TypeCachelookup the old code did inline.The three tables
Each of
SystemMessageSerializer,PersistenceMessageSerializerandPersistenceSnapshotSerializernow overrides
FromBinary(byte[], string)with aDictionary<string, Type>(newTypeExtensions.ManifestTable) covering exactly the types theirFromBinary(byte[], Type)alreadyhandles -
PersistentFSM.PersistentFSMSnapshot<T>is an open generic and stays onbase.FromBinary.An earlier version of this PR keyed the table on a hand-normalized bare type name, which meant every
call ran a regex (
TrySplitTypeName/StripAssemblyIdentity) before the lookup - measured at 465 ns/ 176 B per
FromBinaryversus 56 ns / 24 B beforehand, on a path that includes remote DeathWatchand Persistence recovery in the default configuration. Fixed: each table is keyed by both
Type.TypeQualifiedName()(exactly whatManifest(obj)writes today) and the bareType.FullName,so the common case is a single
Dictionary.TryGetValue- no regex, no allocation. Only on a missdoes it strip assembly identity (one regex pass) and retry once, which is what makes an old or
versioned
AssemblyQualifiedNamespelling still resolve.The guarded fallback, in order
Serializer.FromBinary(byte[], string):TypeCachehit (new non-reflectingTryGetCached) returns immediately - no switch read, sothis never costs anything on a per-message path.
Akka.DynamicTypeLoadingoff throws aSerializationExceptionnaming the manifestand the serializer, and says to resolve the manifest itself or use a
SerializationSetup.[RequiresUnreferencedCode]-attributed private method.TypeCache.GetType/GetTypeInternalalso gained the attribute.Breaking changes
Only with
Akka.DynamicTypeLoadingoff (the default is unaffected - confirmed by every existingserialization/DeathWatch test passing unchanged):
ProtobufSerializermanifest now throws instead of resolving by reflection(tracked separately in AOT: register user Protobuf message types for ProtobufSerializer (no reflection) #8696).
PersistentFSM.PersistentFSMSnapshot<T>now throws a clearSerializationExceptioninstead ofresolving by reflection (or silently succeeding on the JIT).
SerializerwithIncludeManifest = truethat relies on the base class'sTypeCachefallback (rather than overriding
FromBinary(byte[], string)itself) now throws the same clearexception instead of reflecting.
Independent of the switch:
TypeCache.GetType(public) now carries[RequiresUnreferencedCode].A consumer that runs the trim/AOT analyzer on its own code gets a new
IL2026at any call sitethat isn't itself guarded.
Tests
Akka.Tests(SerializerManifestFallbackSpec): a plainSerializersubclass that overridesFromBinary(byte[], string)is actually reached throughSerializerV1Adapter(proves the adapterfix); an unknown manifest throws the clear exception with the switch off; and a manifest already
in
TypeCachestill resolves with the switch off (proves the cache-hit-first order).Akka.Remote.Tests(RemoteSerializersSpec, folded into its existing switch plumbing rather thana third copy of the collection): every
SystemMessagetype round-trips through the realSerialization.Deserialize(bytes, id, manifest)with the switch off, plus two old-styleAssemblyQualifiedNamespellings (one withProcessorArchitecture), so the test can't pass on amerely-warm
TypeCache- it needsSystemMessageSerializer's own table.Akka.Remote.Tests(RemoteDeathWatchSpec): one new end-to-end regression test for what AOT: guard manifest-to-Type resolution (TypeCache) behind Akka.DynamicTypeLoading #8672actually broke - two
ActorSystems over remoting, switch off, aWatchon a remote actor, thenstopping it, must still deliver
Terminated.Akka.Persistence.Tests(PersistenceManifestSpec): every Persistence envelope type plus aSnapshotround-trip through realSerialization.Deserializewith the switch off, a versioned-AQNSnapshotmanifest, andPersistentFSM.PersistentFSMSnapshot<T>throwing.Akka.API.Tests:ApproveCore/ApproveRemote/ApprovePersistence(.DotNet.verified.txt)updated for the new
[RequiresUnreferencedCode]onTypeCache.GetTypeand the newFromBinary(byte[], string)overrides.All of the above pass (folded/new Remote specs and the end-to-end DeathWatch test run 3x clean),
plus the existing
Akka.Tests/Akka.Remote.Tests/Akka.Persistence.Testsserialization suites andevery Remote DeathWatch-related spec (
FullyQualifiedName~Watch, 22 specs) unchanged. Everythingbuilds with
-warnaserror.AOT results
Core canary (
src/aot/Akka.AOT.App), unrooted publish (-p:TrimmerSingleWarn=false):src/core/AkkaTypeCache.GetTypeInternal)[canary] OKon both. Rooted publish (-p:RootAkka=true) plusscripts/CheckAotWarnings.cs:8 in-scope warnings against 8 baseline entries (was 9), no new, none stale - the
TypeCachelineis deleted from
aot-warnings.baseline.txt.Hosting canary (
src/aot/Akka.Hosting.AOT.App): publishes with zero warnings, prints[canary-hosting] OK, andCheckAotWarnings.csagainst its own baseline (src/contrib/hosting/,src/contrib/dependencyinjection/,src/core/Akka.Streams/) reports 0 in-scope warnings.Publish outputs were not committed.