-
Notifications
You must be signed in to change notification settings - Fork 11
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
Conversation
Codecov ReportAttention: Patch coverage is
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. |
There was a problem hiding this 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!
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.
The dictionary will only ever contain items with resolved values. |
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:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks!
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! |
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?