Transients not properly autowired on subsequent get #420

Closed
seancorfield opened this Issue Jan 25, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@seancorfield
Member

seancorfield commented Jan 25, 2016

See aliaspooryorik@3a038bc

@aliaspooryorik says "I think what happens is that when you call getBean on subsequent requests for a transient, it's wiring up the object using the info from variables.accumulatorCache, but doesn't return that instance (or something like that)"

@seancorfield seancorfield self-assigned this Jan 25, 2016

@seancorfield seancorfield added this to the 4.0 milestone Jan 25, 2016

@georgebridgeman

This comment has been minimized.

Show comment
Hide comment
@georgebridgeman

georgebridgeman Jan 25, 2016

From what I have found, the issue is caused by the setters being called on injection.bean but in the case of transients, that reference never updated after it has been instantiated the first time. The first instance is then held in variables.accumulatorCache['myTransient'].injection['myTransient'], and the setters are called on that instance on subsequent calls, but a different instance is returned.

I fixed the issue by putting this code in resolveBeanCreate, just below accumulator.bean = bean; on line 894:

if(!isSingleton(beanName) && structKeyExists(accumulator.injection, beanName)) {
    accumulator.injection[beanName].bean = accumulator.bean;
}

I'm not convinced that this is the correct fix though - smells very hacky - so haven't submitted a PR. I didn't have this issue with previous releases either, so I think the problem is caused elsewhere and my fix merely masks the issue.

From what I have found, the issue is caused by the setters being called on injection.bean but in the case of transients, that reference never updated after it has been instantiated the first time. The first instance is then held in variables.accumulatorCache['myTransient'].injection['myTransient'], and the setters are called on that instance on subsequent calls, but a different instance is returned.

I fixed the issue by putting this code in resolveBeanCreate, just below accumulator.bean = bean; on line 894:

if(!isSingleton(beanName) && structKeyExists(accumulator.injection, beanName)) {
    accumulator.injection[beanName].bean = accumulator.bean;
}

I'm not convinced that this is the correct fix though - smells very hacky - so haven't submitted a PR. I didn't have this issue with previous releases either, so I think the problem is caused elsewhere and my fix merely masks the issue.

@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield Jan 25, 2016

Member

Thanks @georgebridgeman -- that's actually very helpful. The reason this bug has appeared is the caching code added to try to avoid walking dependencies multiple times for transients by not forcing regeneration of the accumulator data, but this seems to be a path through where the newly generated transient needs to be overridden in the cache (although there's still a possibility of multiple transients generated in a single recursive resolution phase where only one of them would have setters called properly... that may be enough of an edge case not to worry about).

Member

seancorfield commented Jan 25, 2016

Thanks @georgebridgeman -- that's actually very helpful. The reason this bug has appeared is the caching code added to try to avoid walking dependencies multiple times for transients by not forcing regeneration of the accumulator data, but this seems to be a path through where the newly generated transient needs to be overridden in the cache (although there's still a possibility of multiple transients generated in a single recursive resolution phase where only one of them would have setters called properly... that may be enough of an edge case not to worry about).

aliaspooryorik added a commit to aliaspooryorik/fw1 that referenced this issue Jan 26, 2016

aliaspooryorik added a commit to aliaspooryorik/fw1 that referenced this issue Jan 26, 2016

aliaspooryorik added a commit to aliaspooryorik/fw1 that referenced this issue Jan 26, 2016

@aliaspooryorik aliaspooryorik referenced this issue Jan 26, 2016

Merged

Issue420 #421

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment