-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Protect NetworkImpl#close
from concurrency issues
#2203
Conversation
@@ -52,17 +53,17 @@ static Network newNetwork() { | |||
@Singular | |||
private Set<Consumer<CreateNetworkCmd>> createNetworkCmdModifiers; | |||
|
|||
private String id; | |||
private final AtomicReference<String> id = new AtomicReference<>(); |
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.
- both
getId
andclose
aresynchronized
, the field can bevolatile
- by making it final, you're breaking the backward compatibility, since (accidentally) the constructor was exposed to public (a class that was intended to be internal became private as it was defined in the interface), please revert
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 see, I thought that NetworkImpl
is internal, but it indeed leaks out through the builder
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.
FYI:
soon we will add japicmp to automate such checks
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.
also, please do not unresolve other's comments.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
NetworkImpl#close
from concurrency issues
@pivovarit I updated the PR and will merge it once CI is green :) |
This was released in https://github.com/testcontainers/testcontainers-java/releases/tag/1.14.0 🎉 Thanks for the contribution! |
Since
NetworkImpl#id
is assigned lazily,close
may not see the change and will fail with NPE.