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

Async versions of ReconfigureTenant, ClearTenants and RemoveTenant #42

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

gliljas
Copy link
Contributor

@gliljas gliljas commented Dec 2, 2024

Since we may have reasons to use DisposeAsync, we should have async versions of these methods as well.

I'll gladly add tests, but wonder what style you prefer if there are sync/async versions. Clone the test? Parametrize?

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.66%. Comparing base (2765221) to head (cbf52b1).
Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
src/Autofac.Multitenant/MultitenantContainer.cs 93.93% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #42      +/-   ##
===========================================
+ Coverage    93.57%   93.66%   +0.08%     
===========================================
  Files            5        5              
  Lines          109      142      +33     
  Branches        28       36       +8     
===========================================
+ Hits           102      133      +31     
  Misses           3        3              
- Partials         4        6       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor doc update and tests needed. I think cloning the associated tests would be better because then we'll get the right name (MethodName_TestScenario) and the tests can be explicitly async Task instead of sync-over-async or vice-versa. Thanks!

src/Autofac.Multitenant/MultitenantContainer.cs Outdated Show resolved Hide resolved

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@gliljas
Copy link
Contributor Author

gliljas commented Dec 4, 2024

There's a sync Dispose in CreateTenantScope, which could mean that there should be a CreateTenantScopeAsync, but I kind of wonder why it creates a new lifetimeScope and then disposes it immediately if one is already in the dictionary. Of course it should be rare, but I wonder if it's not something that could be solved by the good ol' ConcurrentDictionary of Lazy trick.

private ILifetimeScope CreateTenantScope(object tenantId, Action<ContainerBuilder>? configuration = null)
{
    if (_isDisposed == 1)
    {
        throw new ObjectDisposedException(nameof(ApplicationContainer));
    }

    Lazy<ILifetimeScope> lifetimeScope = configuration != null
        ? new Lazy<ILifetimeScope>(() => ApplicationContainer.BeginLifetimeScope(TenantLifetimeScopeTag, configuration))
        : new Lazy<ILifetimeScope>(() => ApplicationContainer.BeginLifetimeScope(TenantLifetimeScopeTag));

    return _tenantLifetimeScopes.GetOrAdd(tenantId, lifetimeScope).Value;
}

The dictionary will only ever contain items with resolved values.

@tillig
Copy link
Member

tillig commented Dec 4, 2024

If you read the comment on the method, the idea was that if, for some reason, the scope couldn't be added to the dictionary due to some sort of limitation, the scope that was just created would be disposed. Like:

  1. Create a scope, which sometimes instantiates or calls back to create things.
  2. Try to add it to the dictionary.
  3. If it wasn't added, dispose of it immediately so the things that may have been created at the time would get cleaned up.

As mentioned in the comment, this should likely never happen.

For now, I'd say let's leave it like it is. It'd be awesome to get the bulk of this PR done and merged in without hanging on this. I'd also say... creating a lifetime scope is a pretty synchronous operation, so I don't know that I'd want to create the scope async or offer a method that makes it appear it's being created async. I'd have to check the code in core Autofac, but I think we end up calling DisposeAsync synchronously in the underlying lifetime scope implementation if plain Dispose is called. Regardless, I'm not sure adding an async create is right, and the rest of this is valuable, so we can maybe address the remainder later.

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@tillig tillig merged commit 4f4b3fb into autofac:develop Dec 4, 2024
3 checks passed
@gliljas
Copy link
Contributor Author

gliljas commented Dec 4, 2024

I agree that CreateTenantScopeAsync is the wrong way to go. A rare case indeed, so not urgent in any way.

Thanks for reviewing and merging!

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