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

Fixing thread safety issue relating to cached setter lookups #46

Merged
merged 1 commit into from Aug 13, 2013

Conversation

mattlevine
Copy link
Contributor

We ran into an issue where transient beans where being returned with their setters not set. It only happened when there were concurrent threads accessing instances of the same bean class.

Thanks for the project!
Matt

@seancorfield
Copy link
Member

Thanx. I took a look and I'm not clear how your patch solves the general thread safety here. Could you explain?

I'm concerned that this might just hide the issue and it will crop up elsewhere.

@mattlevine
Copy link
Contributor Author

In the previous version

var setterMeta = variables.settersInfo[ beanName ];
setterMeta.bean = bean;

The transient bean is set into the persistent struct variables.settersInfo[ beanName ] . This resulted in parallel request for the same beanName stepping on each other. Basically request B would get request A's dependency injection.

As far as I can tell it's isolated to this specific case. The change I'm proposing is actually very similar to how you return values from the cachable() function. It just removes any pass by reference issues.

@seancorfield
Copy link
Member

OK. Let me take another look at what's in that struct and see if I can solve the race condition at source instead, but I may just accept this PR anyway...

@seancorfield
Copy link
Member

I looked at whether to eliminate the settersInfo cache based on findSetters() always returning a new struct instance (which only ever contains a single member, setters) but I can see now why it's worth caching and I see that it isn't a race condition but just a mixing of singleton and transient data. Your patch looks good (apart from formatting :)

seancorfield added a commit that referenced this pull request Aug 13, 2013
Fixing thread safety issue relating to cached setter lookups
@seancorfield seancorfield merged commit 9d697a2 into framework-one:master Aug 13, 2013
@seancorfield
Copy link
Member

Thanx.

seancorfield added a commit to seancorfield/di1 that referenced this pull request Aug 13, 2013
seancorfield added a commit that referenced this pull request Aug 13, 2013
Bump version to 0.4.8 for #46 and fix formatting of that PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants