Skip to content

Make the ClientSession close method thread-safe #1179

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 16, 2023
Merged

Make the ClientSession close method thread-safe #1179

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1909482
Make the ClientSession close method thread-safe
jyemin Aug 15, 2023
9f11b56
Add comment
jyemin Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions driver-core/src/main/com/mongodb/internal/session/BaseClientSessionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import org.bson.BsonDocument;
import org.bson.BsonTimestamp;

import java.util.concurrent.atomic.AtomicBoolean;

import static com.mongodb.assertions.Assertions.assertTrue;
import static com.mongodb.assertions.Assertions.isTrue;

Expand All @@ -39,20 +41,19 @@ public class BaseClientSessionImpl implements ClientSession {
private ServerSession serverSession;
private final Object originator;
private final ClientSessionOptions options;
private final AtomicBoolean closed = new AtomicBoolean(false);
private BsonDocument clusterTime;
private BsonTimestamp operationTime;
private BsonTimestamp snapshotTimestamp;
private ServerAddress pinnedServerAddress;
private BsonDocument recoveryToken;
private ReferenceCounted transactionContext;
private volatile boolean closed;

public BaseClientSessionImpl(final ServerSessionPool serverSessionPool, final Object originator, final ClientSessionOptions options) {
this.serverSessionPool = serverSessionPool;
this.originator = originator;
this.options = options;
this.pinnedServerAddress = null;
closed = false;
}

@Override
Expand Down Expand Up @@ -121,7 +122,7 @@ public BsonTimestamp getOperationTime() {

@Override
public ServerSession getServerSession() {
isTrue("open", !closed);
isTrue("open", !closed.get());
if (serverSession == null) {
serverSession = serverSessionPool.get();
}
Expand All @@ -130,19 +131,19 @@ public ServerSession getServerSession() {

@Override
public void advanceOperationTime(@Nullable final BsonTimestamp newOperationTime) {
isTrue("open", !closed);
isTrue("open", !closed.get());
this.operationTime = greaterOf(newOperationTime);
}

@Override
public void advanceClusterTime(@Nullable final BsonDocument newClusterTime) {
isTrue("open", !closed);
isTrue("open", !closed.get());
this.clusterTime = greaterOf(newClusterTime);
}

@Override
public void setSnapshotTimestamp(@Nullable final BsonTimestamp snapshotTimestamp) {
isTrue("open", !closed);
isTrue("open", !closed.get());
if (snapshotTimestamp != null) {
if (this.snapshotTimestamp != null && !snapshotTimestamp.equals(this.snapshotTimestamp)) {
throw new MongoClientException("Snapshot timestamps should not change during the lifetime of the session. Current "
Expand All @@ -155,7 +156,7 @@ public void setSnapshotTimestamp(@Nullable final BsonTimestamp snapshotTimestamp
@Override
@Nullable
public BsonTimestamp getSnapshotTimestamp() {
isTrue("open", !closed);
isTrue("open", !closed.get());
return snapshotTimestamp;
}

Expand All @@ -182,8 +183,10 @@ private BsonTimestamp greaterOf(@Nullable final BsonTimestamp newOperationTime)

@Override
public void close() {
if (!closed) {
closed = true;
// While the interface implemented by this class is documented as not thread safe, it's still useful to provide thread safety here
// in order to prevent the code within the conditional from executing more than once. Doing so protects the server session pool from
// corruption, by preventing the same server session from being released to the pool more than once.
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

if (serverSession != null) {
serverSessionPool.release(serverSession);
}
Expand Down