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.

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

Refactor type readiness check to avoid NullReferenceException #3121

Merged
merged 5 commits into from Apr 10, 2024

Conversation

Leh2
Copy link
Contributor

@Leh2 Leh2 commented Apr 9, 2024

We encountered a issue where our service would throw a NullReferenceException during startup, necessitating a reboot for recovery. This problem arises particularly when multiple operations execute in parallel at startup, such as Events.AggregateStreamAsync<T> and Events.Append(...) + SaveChanges. This situation triggers the compilation/build of generated types through different execution paths but not in a synchronized manner.

Through debugging, we observed that the setters for our aggregate in the generated document were not set when the error occurred. This led us to scrutinize the compile/build code, particularly the assembleTypes method, where the essential processing happens. We identified a specific flaw where the lock code checks the _liveGeneratedType property, but the setters are assigned afterward. This discrepancy creates a window where the code incorrectly assumes the compilation/build process is complete, proceeding to operations that depend on setters that have not yet been initialized.

The fix involves changing the source of truth for checking if types are ready from _liveType and _liveGeneratedType (which are set mid-process and hence unreliable) to a more dependable flag: _hasGenerated. This change ensures a more accurate and consistent readiness check, preventing the premature usage of types.

Example from our debug where we can see that BuildLiveAggregator is called before the setters are ready ("Setters is now set").

8: Starting building inline projections
8: generateIfNecessary
8: got _assembleLocker
8: Generating
8: tryAttachTypes
8: BuildLiveAggregationType thread
8: _liveGeneratedType is now set
5: BuildLiveAggregator: Pre BuildLiveAggregator called
5: BuildLiveAggregator: _liveType is not null. _liveGeneratedType have value: True
5: BuildLiveAggregator: for Marten.Generated.SingleStreamProjectionLiveAggregation525137164: Setters count 0
8: Setters is now set

The exception:

System.NullReferenceException: Object reference not set to an instance of an object.
   at XxxAggregate Marten.Generated.EventStore.SingleStreamProjectionLiveAggregation792320531.Apply(IEvent event, XxxAggregate aggregate, IQuerySession session) in /source/xxx.Service/Internal/Generated/EventStore/SingleStreamProjectionRuntimeSupport792320531.cs:line 92
   at XxxAggregate Marten.Generated.EventStore.SingleStreamProjectionLiveAggregation792320531.Build(IReadOnlyList<IEvent> events, IQuerySession session, XxxAggregate snapshot) in /source/xxx.Service/Internal/Generated/EventStore/SingleStreamProjectionRuntimeSupport792320531.cs:line 52
   at ValueTask<T> Marten.Events.Aggregation.SyncLiveAggregatorBase<T>.BuildAsync(IReadOnlyList<IEvent> events, IQuerySession session, T snapshot, CancellationToken cancellation)
   at async ValueTask<T> Marten.Events.Aggregation.AggregateVersioning<T>.BuildAsync(IReadOnlyList<IEvent> events, IQuerySession session, T snapshot, CancellationToken cancellation)
   at async Task<T> Marten.Events.QueryEventStore.AggregateStreamAsync<T>(Guid streamId, long version, DateTimeOffset? timestamp, T state, long fromVersion, CancellationToken token)
   at async Task<TAggregate> xxx.RepositoryV2.Load<TAggregate>(Guid id, long version) in /_/Xxx.EventSourcing.MartenV2/RepositoryV2.cs:line 49

Test
I managed to create a reproducible test through a workaround that involved inserting a sleep immediately after setting _liveGeneratedType. This approach also necessitated having events pre-persisted before executing the test to simulate the real-world scenario accurately. While this test is not included, it is accessible for review at the following URL: Leh2@246c83c.

It's important to note that in our implementation, we leverage private apply methods, which exhibit somewhat different behavior in the generated document.

Leh2 added 5 commits April 9, 2024 12:53
Previously, the readiness check relied on the null status of `_liveType` and `_liveGeneratedType`, which proved unreliable as these variables are set mid-process.
@Leh2 Leh2 changed the title Refactor type readiness check to avoid `NullReferenceException Refactor type readiness check to avoid NullReferenceException Apr 9, 2024
@Leh2 Leh2 marked this pull request as ready for review April 9, 2024 13:11
@jeremydmiller jeremydmiller merged commit 36afb2a into JasperFx:master Apr 10, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants