-
Notifications
You must be signed in to change notification settings - Fork 351
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
Fix #3997. Make SupplierFactoryBridge thread-safe. #4000
Conversation
Please see Missing-ECA,-Missing-Sign-off |
I have added "Signed-off-by" entry to new commit, created eclipse account and signed ECA. |
|
inject/hk2/src/main/java/org/glassfish/jersey/inject/hk2/SupplierFactoryBridge.java
Outdated
Show resolved
Hide resolved
inject/hk2/src/main/java/org/glassfish/jersey/inject/hk2/SupplierFactoryBridge.java
Outdated
Show resolved
Hide resolved
synchronized (disposableSuppliers) { | ||
DisposableSupplier<T> disposableSupplier = disposableSuppliers.get(instance); | ||
disposableSupplier.dispose(instance); | ||
disposableSuppliers.remove(instance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is too wide - it synchronizes also the disposal of the supplier
Change this block:
DisposableSupplier<T> disposableSupplier = disposableSuppliers.remove(instance);
disposableSupplier.dispose(instance);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still replace the current implementation with the one I proposed.
I would rather call a single method (remove
) than two methods (get
and remove
) on a synchronized object.
Signed-off-by: MIURA Toru <[email protected]>
@tomas-langer I have changed the code according to your comments. Can you review it again? |
@buri17 I have updated the review comments - one still pending. Thanks and sorry for delay! |
I believe I ran into the same issue. I randomly got an NPE here jersey/inject/hk2/src/main/java/org/glassfish/jersey/inject/hk2/SupplierFactoryBridge.java Line 89 in 01cb903
|
Closing in favor of #4178. |
This makes SupplierFactoryBridge thread-safe and avoids #3997.