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

WFLY-12588 BeanManager.findBean(...) can return an expired bean #12842

Merged
merged 6 commits into from Dec 10, 2019

Conversation

pferraro
Copy link
Contributor

@pferraro pferraro commented Dec 9, 2019

@wildfly-ci wildfly-ci added the deps-ok Dependencies have been checked, and there are no significant changes label Dec 9, 2019
@@ -75,7 +75,7 @@ public I getGroupId() {

@Override
public boolean isExpired() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@pferraro Am I correct in that before this PR this method was never used outside a test case?

I ask because I don't understand the implications of L78 but TBH if Bean.isExpired was never used you can make the semantics what you like and it doesn't matter if I understand. :) (The L78 change doesn't seem wrong, I just don't pretend to deep understanding of the relationship to group.isCloseable vs bean.isExpired.)

The rest of this PR all seems fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

References to Bean.isExpired() were replaced by BeanEntry.isExpired(...) at some point, which meant only having to look up 1 cache entry to determine if a bean is expired. However, a bean should not be considered expired if it is still referenced (i.e. Bean.isCloseable() returns false). A transaction spanning invocations is a prime example of a bean that is not yet closeable.

@bstansberry bstansberry merged commit 752d122 into wildfly:master Dec 10, 2019
@bstansberry
Copy link
Contributor

Thanks @pferraro !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes
Projects
None yet
3 participants