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

Internalize Container synchronization #514

Merged

Conversation

welshm
Copy link
Contributor

@welshm welshm commented Jul 29, 2022

  • Add synchronization tests for Lazy and Producer
  • Fix issue of thread safety not enforced for Lazy and Producer
  • Removes need for SynchronizedResolver by internalizing usage of the RecursiveLock
    • Should be more performant as the synchronization is just done around the final resolution instead of all entry points

Should fix issue #493

- Add synchronization tests for `Lazy` and `Producer`
- Fix issue of thread safety not enforced for `Lazy` and `Producer`
- Removes need for `SynchronizedResolver` by internalizing usage of the `RecursiveLock`
  - Should be more performant as the synchronization is just done around the final resolution instead of all entry points
@welshm
Copy link
Contributor Author

welshm commented Jul 29, 2022

@yoichitgy I would appreciate a review if you have time!

@mpdifran mpdifran self-requested a review August 2, 2022 13:53
Copy link
Member

@mpdifran mpdifran left a comment

Choose a reason for hiding this comment

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

Looks good to me! Would we also need to add let synchronized: Bool to Assembler as well? Or would we do something like this:

myAssembler.resolver.synchronize()

@welshm
Copy link
Contributor Author

welshm commented Aug 2, 2022

Looks good to me! Would we also need to add let synchronized: Bool to Assembler as well? Or would we do something like this:

myAssembler.resolver.synchronize()

I don't think it's needed on the Assembler because the documentation suggests not to synchronize until you've finished registering

/// Returns a synchronized view of the container for thread safety.
/// The returned container is ``Resolver`` type. Call this method after you finish all service registrations
/// to the original container.
///
/// - Returns: A synchronized container as ``Resolver``.

So I think what you've suggested with myAssembler.resolver.synchronize() would be the correct approach - unless there is a goal to synchronize assembly as well.

@mpdifran mpdifran requested a review from maxim-chipeev August 2, 2022 17:48
@mpdifran mpdifran merged commit d539168 into Swinject:master Aug 2, 2022
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.

3 participants