Skip to content

Comments

fix: remove empty Dispose and fix nullability annotation#4920

Merged
thomhurst merged 1 commit intomainfrom
fix/resource-management
Feb 19, 2026
Merged

fix: remove empty Dispose and fix nullability annotation#4920
thomhurst merged 1 commit intomainfrom
fix/resource-management

Conversation

@thomhurst
Copy link
Owner

Summary

Closes #4858

  • Remove unnecessary IDisposable implementation from EventReceiverOrchestrator: The Dispose() method was empty (the underlying EventReceiverRegistry no longer uses ReaderWriterLockSlim). No callers explicitly call Dispose() on this type -- it was only invoked by TUnitServiceProvider.DisposeAsync() which iterates registered services and disposes any that implement IDisposable. Removing the interface means the service provider simply skips it during cleanup.
  • Add nullable annotation to PropertyInjector.InjectPropertiesRecursiveAsync: The instance parameter was typed as non-nullable object but had a null guard that returned early. This is correct behavior for a recursive method (nested property values can be null), so the parameter is now annotated as object? to match the actual contract.

Test plan

  • dotnet build TUnit.Engine/TUnit.Engine.csproj succeeds with 0 warnings and 0 errors across all target frameworks (netstandard2.0, net8.0, net9.0, net10.0)
  • CI passes

@claude
Copy link
Contributor

claude bot commented Feb 19, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Both changes are well-scoped and correct:

EventReceiverOrchestrator: Remove IDisposable
The empty Dispose() removal is safe — EventReceiverRegistry no longer holds a ReaderWriterLockSlim (or any other disposable resource), so there is nothing to clean up. TUnitServiceProvider.DisposeAsync() does a runtime is IDisposable check before calling Dispose(), so skipping this class during teardown is the right outcome.

PropertyInjector: objectobject?
The nullability annotation now matches the method's long-standing behavior. The null guard at the top of InjectPropertiesRecursiveAsync already handled null inputs; annotating the parameter accordingly eliminates a misleading non-nullable contract on what is effectively a nullable-accepting recursive helper. The public InjectPropertiesAsync entry point still enforces non-null via ArgumentNullException, so the public API contract is unchanged.

Remove unnecessary IDisposable implementation from EventReceiverOrchestrator
since the Dispose method was empty (the underlying registry no longer uses
ReaderWriterLockSlim). The service provider skips non-IDisposable services
automatically.

Add nullable annotation to the `instance` parameter in
PropertyInjector.InjectPropertiesRecursiveAsync, since the method handles
null by returning early (correct for recursive traversal of nested objects).
@thomhurst thomhurst force-pushed the fix/resource-management branch from f0026fa to be62d46 Compare February 19, 2026 11:47
@thomhurst thomhurst merged commit 68c3559 into main Feb 19, 2026
13 of 14 checks passed
@thomhurst thomhurst deleted the fix/resource-management branch February 19, 2026 13:51
This was referenced Feb 23, 2026
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.

fix: resource management issues - empty Dispose and missing nullability

1 participant