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.

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

Fix ActivityID map getting not cleaned up properly #1018

Closed
wants to merge 2 commits into from
Closed

Fix ActivityID map getting not cleaned up properly #1018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e33028
fix issue
peterbae Apr 2, 2019
fe7030c
this doesnt need public
peterbae Apr 2, 2019
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
14 changes: 9 additions & 5 deletions src/main/java/com/microsoft/sqlserver/jdbc/ActivityCorrelator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,8 @@ final class ActivityCorrelator {

private static Map<Long, ActivityId> activityIdTlsMap = new ConcurrentHashMap<>();

static void cleanupActivityId() {
static void cleanupActivityId(long uniqueThreadId) {
// remove the ActivityId that belongs to this thread.
long uniqueThreadId = Thread.currentThread().getId();

if (activityIdTlsMap.containsKey(uniqueThreadId)) {
activityIdTlsMap.remove(uniqueThreadId);
}
Expand All @@ -33,7 +31,7 @@ static ActivityId getCurrent() {

// Since the Id for each thread is unique, this assures that the below if statement is run only once per thread.
if (!activityIdTlsMap.containsKey(uniqueThreadId)) {
activityIdTlsMap.put(uniqueThreadId, new ActivityId());
activityIdTlsMap.put(uniqueThreadId, new ActivityId(uniqueThreadId));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Anywhere that calls getCurrent() is going to end up putting an entry in the map in the context of the current thread. In a pooled environment, each thread that uses the connection could potentially do that and then only the pooling thread that closes the connection will remove the original thread's entry from the map. There will still be a leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'm looking into the documentation as to if Activity IDs are tied to a connection (one ActivityID per connection) or if it's tied to other queries as well. Depending on that, I think we can come up with a solution that doesn't affect the driver's performance - I'll come up with an update soon.

}

return activityIdTlsMap.get(uniqueThreadId);
Expand Down Expand Up @@ -65,14 +63,20 @@ private ActivityCorrelator() {}

class ActivityId {
private final UUID id;
private final long uniqueThreadId;
private long sequence;
private boolean isSentToServer;

ActivityId() {
ActivityId(long uniqueThreadId) {
id = UUID.randomUUID();
this.uniqueThreadId = uniqueThreadId;
sequence = 0;
isSentToServer = false;
}

long getUniqueThreadId() {
return uniqueThreadId;
}

UUID getId() {
return id;
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ public class SQLServerConnection implements ISQLServerConnection, java.io.Serial
static final int DEFAULT_SERVER_PREPARED_STATEMENT_DISCARD_THRESHOLD = 10; // Used to set the initial default, can
// be changed later.
private int serverPreparedStatementDiscardThreshold = -1; // Current limit for this particular connection.
private long activityIdUniqueThreadId = -1;

/**
* The default for if prepared statements should execute sp_executesql before following the prepare, unprepare
Expand Down Expand Up @@ -2542,6 +2543,7 @@ void Prelogin(String serverName, int portNumber) throws SQLServerException {
String preloginErrorLogString = " Prelogin error: host " + serverName + " port " + portNumber;

ActivityId activityId = ActivityCorrelator.getNext();
activityIdUniqueThreadId = activityId.getUniqueThreadId();
final byte[] actIdByteArray = Util.asGuidByteArray(activityId.getId());
final byte[] conIdByteArray = Util.asGuidByteArray(clientConnectionId);

Expand Down Expand Up @@ -3242,7 +3244,7 @@ private void clearConnectionResources() {
// Clean-up queue etc. related to batching of prepared statement discard actions (sp_unprepare).
cleanupPreparedStatementDiscardActions();

ActivityCorrelator.cleanupActivityId();
ActivityCorrelator.cleanupActivityId(activityIdUniqueThreadId);
}

// This function is used by the proxy for notifying the pool manager that this connection proxy is closed
Expand All @@ -3261,12 +3263,11 @@ final void poolCloseEventNotify() throws SQLServerException {
connectionCommand("IF @@TRANCOUNT > 0 ROLLBACK TRAN" /* +close connection */, "close connection");
}
notifyPooledConnection(null);
ActivityCorrelator.cleanupActivityId();
ActivityCorrelator.cleanupActivityId(activityIdUniqueThreadId);
if (connectionlogger.isLoggable(Level.FINER)) {
connectionlogger.finer(toString() + " Connection closed and returned to connection pool");
}
}

}

@Override
Expand Down