Fix thread leak #568

littleaj · 2018-02-22T21:03:39Z

Fix #513 .

NOTE: This fix changes the lowest version of the Servet API we support from 2.5 to 3.0!

Here's the gist of the issue:

When our TelemetryClient loads it starts a bunch of threads.
It registers a shutdown hook with the JVM
When an application using our TelemetryClient is undeployed, the app server expects our threads to stop, but since the JVM isn't stopping, the threads continue to run.

Here's how I fixed it:

I added names to the threads we create. Added AtomicInteger for tracking thread pool names.
I changes the logging format to include the thread name after the thread id.
I added a WebListener which kills our threads when the webapp is shutdown/undeployed.

Here's some other things you'll find in this PR:

added some additional scripts for smoketests to finish debugging.
Annotated our filter with the Servlet 3.0 annotations for filters

For significant contributions please make sure you have completed the following items:

Test this in SpringBoot App
Changes in public surface reviewed
CHANGELOG.md updated

added thread name to loggers. registered CdsProfileFetcher to be shutdown by sdk mechanism. upgraded to Servlet 3.0.1 spec. Added listener to trigger shutdown during undeploy. added undeploy.sh so tests can be written for this scenario.

increased the number of lines to tail from 25 to 50. added option to specify a different log file. added usage on error from tail command.

fixed command

change 'ADD' directive to use wildcard.

dhaval24

@littleaj one general comment which applies to all Atomic Integer variables. Why should they not be static? I believe they should be shared across different instance of classes so that ThreadId values do not get duplicated. Apart from this left other comments.

dhaval24 · 2018-02-23T06:15:05Z

.../src/main/java/com/microsoft/applicationinsights/internal/channel/common/ApacheSender42.java

@@ -37,13 +37,14 @@
 */


I have a fundamental question. Since we are internally using Apache HTTP Client 4.5.3 to send the data, why do we need this sender in the first place? I think if there is anywhere still we are using HTTP Client 4.2 we should remove those. For eg ApacheSenderFactory looks for old jar on the class path and creates an instance of ApacheSender42. I think we do not need it at all. @littleaj can you take a look as a part of this improvements and trim the unnecessary code there ?

That's something we can address separately. If it's worth exploring, we should file an issue.

Please file an issue to keep an eye on. We can then close if not needed.

dhaval24 · 2018-02-23T06:23:45Z

core/src/main/java/com/microsoft/applicationinsights/internal/shutdown/SDKShutdownActivity.java

-    private SDKShutdownThread getShutdownThread() {
-        if (shutdownThread == null) {
+    private SDKShutdownAction getShutdownAction() {
+        if (shutdownAction == null) {
            synchronized (this) {


@littleaj As a one cent advise, I would prefer to start a practice of using Reentrant Lock instead of synchronizing on this. There are numerous benefits . I would like everyone to go over.
https://stackoverflow.com/questions/11821801/why-use-a-reentrantlock-if-one-can-use-synchronizedthis
Also it is much more cleaner and less prone to errors.

I don't see any benefits of using RentrantLock. IMO, Java's lock classes are much less readable and harder to debug, but I'm happy to evaluate some evidence why synchronized is inadequate for this case.

However, (quoting the best answer from the linked stackoverflow article):

... use it when you actually need something it provides that synchronized doesn't ... but remember that the vast majority of synchronized blocks hardly ever exhibit any contention, let alone high contention. I would advise developing with synchronization until synchronization has proven to be inadequate, rather than simply assuming "the performance will be better" if you use ReentrantLock. Remember, these are advanced tools for advanced users. (And truly advanced users tend to prefer the simplest tools they can find until they're convinced the simple tools are inadequate.) ...

Ok, I have no objections. I personally feel writing synchronized is two step process whereas reentrant lock just makes it more readable and easy.

dhaval24 · 2018-02-23T06:24:47Z

web/build.gradle

@@ -36,7 +36,7 @@ dependencies {
    compile ([group: 'org.apache.httpcomponents', name: 'httpasyncclient', version: '4.1.3'])
    provided 'com.opensymphony:xwork:2.0.4' // Struts 2
    provided 'org.springframework:spring-webmvc:3.1.0.RELEASE'
-    provided group: 'javax.servlet', name: 'servlet-api', version: '2.5'
+    provided group: 'javax.servlet', name: 'javax.servlet-api', version: '3.0.1'


Please make sure to register this on OSS before merging.

Pending review

dhaval24 · 2018-02-23T06:27:23Z

.../main/java/com/microsoft/applicationinsights/web/internal/correlation/AppProfileFetcher.java

 import org.apache.http.ParseException;

 /**
 * Retrieves the application profile from storage
 */
- public interface AppProfileFetcher {
+ public interface AppProfileFetcher extends Closeable {


For this @littleaj could you please confirm if this doesn't break spring boot scenario. Specifically I would like to see that with this change do we still need to register the Bean for WebRequestTrackingFilter or the framework can automatically pick it?

Sounds good. I'll see what I can find out.

dhaval24 · 2018-02-23T06:28:00Z

.../main/java/com/microsoft/applicationinsights/web/internal/correlation/CdsProfileFetcher.java

@@ -39,7 +46,7 @@

 public class CdsProfileFetcher implements AppProfileFetcher {

-	private HttpAsyncClient httpClient;
+	private CloseableHttpAsyncClient httpClient;


why shouldn't this variable be final? Do we ever need to do multiple initialization for this?

The setter is public

Sure in this case it cannot be fine but atleast @littleaj the setter can be set to package private (aka default access). This setter is only used in CdsProfileFetcherTests.java which resides in the same package. I know this is not part of your PR but it's good if we can address if possible in this PR.

@dhaval24 I don't think we can reduce the visibility of a public method in a public class w/o going through the deprecation procedure.

littleaj · 2018-02-23T21:37:13Z

@dhaval24 Regarding your comment about AtomicInteger. It does appear to be possible to have multiple instances of some of these classes, so I've refactored the ThreadFactory creation to include and instance identifier.

dhaval24 · 2018-02-24T17:57:18Z

@littleaj yes it doesn't appear to have the multiple instances of the classes, but there is no restriction in code base that it would prevent a developer to make that mistake in the future. Either we should add those restrictions on make sure that ThreadIds you are providing are unique always. May be the logic you added in your recent commit will ensure that.

littleaj · 2018-02-27T01:43:01Z

@dhaval24 The instance identifier does ensure the pools won't have colliding IDs.

dhaval24

@littleaj I verified your changes for the SpringBoot Scenario. We would no longer need to specify FilterRegistrationBean and Bean for WebRequestTrackingFilter. Instead with this change the user will have to annotate the SpringBoot main class with @ServletComponentScan("com.microsoft.applicationinsights.web.internal") tag so that Spring auto configuration can pick up the filter from the Microsoft package. Also the current users who used the bean registration users won't get affected (i.e broken) if they update the library. Both the methods should work fine. Please make sure to update the SpringBoot Documentation to include the same.

PS: SpringBoot needs this additional tag to pick up the Servlet annotations because of embedded servlet containers it uses.

dhaval24 · 2018-03-09T02:27:19Z

@littleaj really sorry for the later reply. But I found an issue with SpringBoot.I was experimenting more with this and when I instrumented it with Agent I found that we won't be able to use the parameterized constructor in web request tracking filter to bind the name to the filter.

Therefore, this won't eliminate creating the two beans I mentioned in my above comment, but would be mandatory.

This won't block your PR though, just there would be no advantage in case of SpringBoot. Apologies again.

littleaj added 8 commits February 15, 2018 12:50

named all our threads.

850a1c2

added thread name to loggers. registered CdsProfileFetcher to be shutdown by sdk mechanism. upgraded to Servlet 3.0.1 spec. Added listener to trigger shutdown during undeploy. added undeploy.sh so tests can be written for this scenario.

Merge branch 'master' into shutdownhook_leak

0ce99a5

fixed compile error; added close()

416edd4

explicitly called a log file (ls wasn't picking the correct one).

9f1fe54

increased the number of lines to tail from 25 to 50. added option to specify a different log file. added usage on error from tail command.

fixed log messages from undeploy.sh (copy paste error)

6f092ff

fixed command

updated docker file to include the package with 'ps' command.

34de317

change 'ADD' directive to use wildcard.

added startServer and stopServer scripts for debugging

0e0d8c6

reverted Thread.sleep changes; superfluous

48105f8

littleaj self-assigned this Feb 22, 2018

littleaj requested review from grlima and dhaval24 February 22, 2018 21:03

dhaval24 suggested changes Feb 23, 2018

View reviewed changes

littleaj added 2 commits February 23, 2018 14:12

added release note

de6f60a

refactored ThreadFactory creation to be a bit more readable.

c581ec2

littleaj added this to the 2.0.1 milestone Feb 23, 2018

grlima approved these changes Feb 27, 2018

View reviewed changes

littleaj modified the milestones: 2.0.1, 2.0.2 Mar 2, 2018

dhaval24 approved these changes Mar 4, 2018

View reviewed changes

Merge branch 'master' into shutdownhook_leak

6e3f447

dhaval24 approved these changes Mar 7, 2018

View reviewed changes

This was referenced Mar 7, 2018

Add additional SpringBoot instructions for filter after Servlet upgrade #595

Closed

Update general onboarding instructions after upgrading to Servlet 3.0 #596

Closed

littleaj merged commit d43e85d into master Mar 7, 2018

littleaj deleted the shutdownhook_leak branch March 7, 2018 22:56

littleaj mentioned this pull request Aug 7, 2018

ApplicationInsights-Java having issues with Tomcat Shutdown. #712

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix thread leak #568

Fix thread leak #568

littleaj commented Feb 22, 2018 •

edited

Loading

dhaval24 left a comment

dhaval24 Feb 23, 2018

littleaj Feb 23, 2018 •

edited

Loading

dhaval24 Feb 24, 2018

dhaval24 Feb 23, 2018

littleaj Feb 23, 2018 •

edited

Loading

dhaval24 Feb 24, 2018

dhaval24 Feb 23, 2018

littleaj Feb 23, 2018

dhaval24 Feb 23, 2018

littleaj Feb 24, 2018

dhaval24 Feb 23, 2018

littleaj Feb 23, 2018

dhaval24 Feb 24, 2018

littleaj Mar 2, 2018

littleaj commented Feb 23, 2018

dhaval24 commented Feb 24, 2018

littleaj commented Feb 27, 2018 •

edited

Loading

dhaval24 left a comment •

edited by littleaj

Loading

dhaval24 commented Mar 9, 2018

Fix thread leak #568

Fix thread leak #568

Conversation

littleaj commented Feb 22, 2018 • edited Loading

dhaval24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littleaj Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littleaj Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littleaj commented Feb 23, 2018

dhaval24 commented Feb 24, 2018

littleaj commented Feb 27, 2018 • edited Loading

dhaval24 left a comment • edited by littleaj Loading

Choose a reason for hiding this comment

dhaval24 commented Mar 9, 2018

littleaj commented Feb 22, 2018 •

edited

Loading

littleaj Feb 23, 2018 •

edited

Loading

littleaj Feb 23, 2018 •

edited

Loading

littleaj commented Feb 27, 2018 •

edited

Loading

dhaval24 left a comment •

edited by littleaj

Loading