Thread consuming problems using sendRequestAsync() call

[norbertroamsys]

We use the latest version of this library:

        <dependency>
            <groupId>org.piwik.java.tracking</groupId>
            <artifactId>matomo-java-tracker</artifactId>
            <version>1.5</version>
        </dependency> 

Because of a deprecated message we changed the PiwikTracker.sendRequest() to the asynchronous version PiwikTracker.sendRequestAsync().

After a deployment at the live systems we noticed a large consumption of threads: The number of threads grow on every page request by the number of CPUs + 1! After a longer research we found the reason within your sendRequestAsync() implementation: This method creates a new AsyncHttpClient each time. As a consequence a new threadpool is created.

Please note that the documentation of AHC clearly defines that this kind of client should be a singleton / should be shared within the application:

AsyncHttpClient instances are intended to be global resources that share the same lifecycle as the application. Typically, AHC will usually underperform if you create a new client for each request, as it will create new threads and connection pools for each. It's possible to create shared resources (EventLoop and Timer) beforehand and pass them to multiple client instances in the config. You'll then be responsible for closing those shared resources.

We changed our code back for using the synchronous version to fix the problem by now.

Suggested solutions:

• Share the client by using a singleton. In this case we do not need to change the code.
• Create the client once in PiwikTracker. In this case we will change the code to use PiwikTracker as a singleton.

[tholu]

@norbertroamsys Thanks for your detailed report! This has been solved in #43 (see #42). I have to release a new version.

Can you confirm the fix by trying the latest master via jitpack.io?

    <repositories>
        <repository>
            <id>jitpack.io</id>
            <url>https://jitpack.io</url>
        </repository>
    </repositories>

        <dependency>
        <groupId>com.github.matomo-org</groupId>
        <artifactId>matomo-java-tracker</artifactId>
        <version>master-SNAPSHOT</version>
    </dependency>

[tholu]

@norbertroamsys Release of v1.6 to Maven Central is in progress as well, should be available in a few hours. Let me know if that fixes the problem and feel free to reopen the issue if it doesn't!

[norbertroamsys]

Hi Thomas, thanks for the swift reply! In the morning I had a look on open tickets - but not on the closed too. My fault ;-). I also checked Maven central repository for newer version. I will check again tomorrow and let you know if it is fixed or not. But I am pretty sure the problem is solved by your code changes (which are 100% what I suggested as second option).

[tholu]

Sure, no problem - thanks for reminding me that a release should be done immediately. 1.6 is now available on Maven Central.

[norbertroamsys]

Hi Thomas, we now use v1.6 and can confirm that the bug is fixed: Only one thread pool per PiwikTracker instance is created.

So thanks again!

But let me just leave a critical comment about the usage: I think it is not self explaining that a consumer has to take care to use a singleton of the PiwikTracker in his code. Otherwise one will run into the same issue as in v1.5 which is really a bad situation! We searched for days to find the reason for the OutOfMemory raised by the high number of threads/processes/open files. At least the API documentation should clarify this (as Apache does in the AHC documentation).

[tholu]

@norbertroamsys Critical comments like these are welcome! Would it be possible to contribute a Pull Request to improve the API documentation in this regard?

[norbertroamsys]

@tholu I hope you are also open for a pull request containing an improvement that will prevent the misuse: https://github.com/matomo-org/matomo-java-tracker/pull/52 In my opinion this will help more than an API documentation extension.

I just moved the code of creating clients to an internally factory. AsyncHttpClient instances are created and cached as singletons. HttpClient instances are created on each call. Of course I am open to change the details of the requested changes.

Looking forward to your review ;-)...

[norbertroamsys]

Hi @tholu! I want to follow up after 3 weeks w/o feedback ;-).

Any thoughts regarding my changes?

[tholu]

@norbertroamsys Thanks for the reminder! I'm definitely open to a pull request, however, I did not have time to review it yet. It's on my to-do list 👍