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

Make the ClientSession close method thread-safe #1179

Merged
merged 2 commits into from
Aug 16, 2023
Merged

Conversation

jyemin
Copy link
Collaborator

@jyemin jyemin commented Aug 15, 2023

Replace the volatile boolean with an AtomicBoolean to ensure that the side effects of the close method only execute once.

JAVA-5107

Replace the volatile boolean with an AtomicBoolean
to ensure that the side effects of the close method
only execute once.

JAVA-5107
@jyemin jyemin self-assigned this Aug 15, 2023
@jyemin jyemin requested a review from stIncMale August 15, 2023 17:31
@stIncMale
Copy link
Member

I have two concerns:

  1. If we do the change, it may be worth explaining "why" in the code. Seeing code that deals with thread-safety in a class which must not be used concurrently is surprising.
  2. This change patches one hole, but there are more. Both ClientSessionImpl and BaseClientSessionImpl have mutable plain fields, and do plain reads from / writes to them. If application code shares a session with multiple threads, and starts using it concurrently from those threads, that may lead to the session becoming corrupted (data races in Java are not UB, but are still bad, and race conditions are not a good thing either). Are we pathing this specific hole because the corruption it may cause spreads outside of a specific session?

@jyemin
Copy link
Collaborator Author

jyemin commented Aug 15, 2023

Are we patching this specific hole because the corruption it may cause spreads outside of a specific session?

Yes, as it causes persistent corruption of the session pool.

@@ -182,8 +183,7 @@ private BsonTimestamp greaterOf(@Nullable final BsonTimestamp newOperationTime)

@Override
public void close() {
if (!closed) {
closed = true;
if (closed.compareAndSet(false, true)) {
Copy link
Member

Choose a reason for hiding this comment

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

[optional]
It will be helpful to have a comment here explaining why synchronization exists in a class, instances of which must not be used concurrently. Such a comment may address

  • why concurrency is relevant for instances of this class at all
  • what specific problem is solved here
  • why problems that concurrency may cause in other places of this class are disregarded

Copy link
Member

@stIncMale stIncMale left a comment

Choose a reason for hiding this comment

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

Approving because the only suggestion I left is optional.

@jyemin jyemin merged commit 80a9cb4 into mongodb:master Aug 16, 2023
@jyemin jyemin deleted the j5107 branch August 16, 2023 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants