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.

Sign up for GitHub

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

Jump to bottom

Castle.Windsor.Extensions.DependencyInjection: support parallel containers (see #563) #577

Merged
merged 28 commits into from
Dec 16, 2020
Merged

Castle.Windsor.Extensions.DependencyInjection: support parallel containers (see #563) #577

merged 28 commits into from
Dec 16, 2020

Conversation

rvdginste
Copy link
Contributor

An attempt to fix the issue in #563.

@rvdginste rvdginste mentioned this pull request Nov 30, 2020
Closed
@generik0
Copy link
Contributor

generik0 commented Dec 1, 2020 •
edited

@rvdginste @jonorossi
Reviewed. Looks great, thanks.

@rvdginste can you give me access to the fork? I have a test to add for this change that tests the muli assembly issues easily.

@castleproject castleproject deleted a comment from jonorossi Dec 1, 2020
@rvdginste rvdginste force-pushed the feature/support-parallel-containers branch from 5194020 to 4c4a616 Compare December 1, 2020 12:04
@rvdginste
Copy link
Contributor Author

@rvdginste can you give me access to the fork? I have a test to add for this change that tests the muli assembly issues easily.

@generik0 I sent you an invite.. not clear whether I still have to give you write access though, just let me know.

@generik0
Copy link
Contributor

generik0 commented Dec 2, 2020 •
edited

@rvdginste good work.
I still don't know why I didn't catch the scope factory, as of cause we should control from the factory...! But sometimes one just goes down the wrong track. Thanks for getting the change back on track 👍
@jonorossi I have added uTest that used to fail before due to the root scope being statically bound to the test assembly, but now it is bound to the provider factory instance. Much better.
The test also documents how you can add your own container to the mix, allow for extra control and registrations.

If the tests pass in a bit, then I think this change is good to merge.

@rvdginste rvdginste force-pushed the feature/support-parallel-containers branch from c838b20 to d46be5b Compare December 2, 2020 16:10
jonorossi
jonorossi requested changes Dec 3, 2020
View reviewed changes
Copy link
Member

@jonorossi jonorossi left a comment

Choose a reason for hiding this comment

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

Thanks @rvdginste. I've included some review comments. Could you please also update the changelog.

@jonorossi jonorossi linked an issue Dec 4, 2020 that may be closed by this pull request
Castle.Windsor.Extensions.DependencyInjection parallelism issues #563
Closed
@jonorossi jonorossi mentioned this pull request Dec 4, 2020
Closed
jonorossi
jonorossi requested changes Dec 9, 2020
View reviewed changes
Copy link
Member

@jonorossi jonorossi left a comment

Choose a reason for hiding this comment

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

Sorry @generik0 for the back and forth on this pull request, I've got a few more review comments on the recent commits.

src/Castle.Windsor.Extensions.DependencyInjection/Scope/ExtensionContainerScope.cs Outdated Show resolved Hide resolved
src/Castle.Windsor.Extensions.DependencyInjection/Scope/ExtensionContainerScope.cs Outdated Show resolved Hide resolved
src/Castle.Windsor.Extensions.DependencyInjection/Scope/ExtensionContainerScopeAccessor.cs Outdated Show resolved Hide resolved
src/Castle.Windsor.Extensions.DependencyInjection/WindsorScopedServiceProvider.cs Outdated Show resolved Hide resolved
src/Castle.Windsor.Extensions.DependencyInjection/WindsorScopedServiceProvider.cs Show resolved Hide resolved
@generik0
Rename CurrentOrThrow to Current
ecfa403 
castleproject#563
@generik0
Add check that the disposing scope is current, before changing the pa…
36ac910 
…rent.

castleproject#563
@generik0
Move the set of the curr3ent.Value to the ctor from the ExtensionCont…
3a3a63e 
…ainerRoot

castleproject#563
@generik0
Merge remote-tracking branch 'castle.windsor/master' into feature/sup…
bdf3507 
…port-parallel-containers
@generik0
Improve ordering for readability
32ce9c5 
castleproject#563
@generik0
Rename ExtensionContainerScope to Base
f6740c4 
castleproject#563
@generik0
Make ExtensionContainerRootScope and ExtensionContainerRoot use a base
414887d 
castleproject#563
@generik0
Change BeginScope and BeginRootScope to set current
e039c17 
castleproject#563
@generik0
Move Current into its own static class
5c7f535 
castleproject#563
@generik0
Cleanup
6d4d75e 
castleproject#563
@generik0
Cleanup
b9753fb 
castleproject#563
@generik0
Copy link
Contributor

@jonorossi i cleaned up, and improved substitution and single responsibility a bit:

  • ExtensionContainerScopeBase handles common code for ExtensionContainerScope and ExtensionContainerRootScope now
  • ExtensionContainerScopeCache is now responsible for caching the scoping

I can revert if you do not feel i have SOLIDfied the Castle.Windsor.Extensions.DependencyInjection more

@jonorossi jonorossi added this to the vNext milestone Dec 16, 2020
@jonorossi jonorossi merged commit 14089ae into castleproject:master Dec 16, 2020
@jonorossi jonorossi mentioned this pull request Jul 23, 2023
Open
rvdginste added a commit to rvdginste/Windsor that referenced this pull request Aug 6, 2023
@rvdginste
Fix castleproject#646: fix scope and scope-accessor implementation
464595b 
To fix issue castleproject#563 (support concurrently existing containers) a fix was
created in castleproject#577.  After the initial fix some refactorings were done on
the scope implementation that were not correct.

This commit changes the scope implementation back to the original
implementation of @ltines, but with the support for concurrent
containers.
rvdginste added a commit to rvdginste/Windsor that referenced this pull request Aug 6, 2023
@rvdginste
Fix castleproject#646: fix scope and scope-accessor implementation
1203337 
To fix issue castleproject#563 (support concurrently existing containers) a fix was
created in castleproject#577.  After the initial fix some refactorings were done on
the scope implementation that were not correct.

This commit changes the scope implementation back to the original
implementation of @ltines, but with the support for concurrent
containers.
gavinschultz pushed a commit to spectraqest/Windsor that referenced this pull request Sep 12, 2023
@rvdginste @gavinschultz
Fix castleproject#646: fix scope and scope-accessor implementation
b65b43f 
To fix issue castleproject#563 (support concurrently existing containers) a fix was
created in castleproject#577.  After the initial fix some refactorings were done on
the scope implementation that were not correct.

This commit changes the scope implementation back to the original
implementation of @ltines, but with the support for concurrent
containers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@generik0 generik0 generik0 approved these changes

@jonorossi jonorossi jonorossi requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v6.0.0
Development

Successfully merging this pull request may close these issues.

Castle.Windsor.Extensions.DependencyInjection parallelism issues
3 participants
@rvdginste @generik0 @jonorossi