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

Change Activity ID behavior #2136

Merged
merged 3 commits into from
Jun 6, 2023
Merged

Conversation

David-Engel
Copy link
Contributor

@David-Engel David-Engel commented Jun 1, 2023

  • Activity ID stays the same for the life of the process (not per thread)
  • Always send Activity ID to the server in PRELOGIN
  • Increment the sequence for each new connection (not each command)

Also send a unique client ID (netAddress) that will persist for the duration of the process.

@David-Engel David-Engel changed the title Change Activity ID behavior: Change Activity ID behavior Jun 1, 2023
@lilgreenbird lilgreenbird added this to In progress in MSSQL JDBC via automation Jun 2, 2023
@lilgreenbird lilgreenbird added this to the 12.3.1 milestone Jun 2, 2023
- Activity ID stays the same for the life of the process (not per thread)
- Always send Activity ID to the server in PRELOGIN
- Increment the sequence for each new connection (not each command)
@Jeffery-Wasty Jeffery-Wasty moved this from In progress to Under Peer Review in MSSQL JDBC Jun 2, 2023
@lilgreenbird
Copy link
Member

/azp run CI-mssql-jdbc

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@lilgreenbird lilgreenbird merged commit 953e4a0 into microsoft:main Jun 6, 2023
21 of 22 checks passed
MSSQL JDBC automation moved this from Under Peer Review to Closed/Merged PRs Jun 6, 2023
@David-Engel David-Engel deleted the ActivityID branch August 1, 2023 22:57
@FlyingWalross
Copy link

@David-Engel

Hi quick question about this, the changes in #2254 seem to reverse the changes made in #465 (see also #314) that fixed a ClassLoader leak that was a result of the ActivityCorrelator ThreadLocal never being removed from the thread. Now the ThreadLocal seems to have been added back:

final class ActivityCorrelator {

+    private static ThreadLocal<ActivityId> t_ActivityId = new ThreadLocal<ActivityId>() {
+        @Override
+        protected ActivityId initialValue() {
+            return new ActivityId();

Since this ThreadLocal is now being created again, I am seeing a ClassLoader leak when unloading a webapp using the mssql-jdbc-12.6.0.jre8.jar driver:

SEVERE: The web application [...] created a ThreadLocal with key of type [com.microsoft.sqlserver.jdbc.ActivityCorrelator$1] (value [com.microsoft.sqlserver.jdbc.ActivityCorrelator$1@29887163]) and a value of type [com.microsoft.sqlserver.jdbc.ActivityId] (value [...]) but failed to remove it when the web application was stopped.

The same does not happen with the mssql-jdbc-12.4.0.jre8.jar driver, which was released before this change. I am properly deregistering the driver on webapp unload, but I just cannot seem to get rid of this ClassLoader leak. Using a profiler it shows the GC root of the webapp ClassLoader to be the ActivityCorrelator in the ThreadLocal map of the main thread.

Do you or anyone familiar with this issue have any ideas on how I could solve this? Unfortunately we frequently need to reload webapps and leaking a few hundred mb of memory due to the ClassLoader leak is not really an option, so we are currently having to stick to the older driver version.

Any help would be much appreciated, thanks!

@lilgreenbird
Copy link
Member

hi @FlyingWalross

thanks for reporting this we will work on addressing this issue.

@FlyingWalross
Copy link

@lilgreenbird Thanks for the quick response and fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

None yet

5 participants