[GitHub] [maven-mvnd] psiroky commented on issue #798: 0.10.0-SNAPSHOT is leaking heap memory via ThreadLocal

["psiroky (via GitHub)" <gi...@apache.org>]

psiroky commented on issue #798:
URL: https://github.com/apache/maven-mvnd/issues/798#issuecomment-1455809665

   Here are some more details from my investigation:
    * the `ThreadLocal` instances are held by the `Thread-0` thread, which is the main "accept" thread https://github.com/apache/maven-mvnd/blob/c65f9fe869c902cf03f859f383b2bbfe2211ff8b/daemon/src/main/java/org/mvndaemon/mvnd/daemon/Server.java#L216 This thread waits for messages (build requests) from the client and then runs the Maven build. As far as I can tell this thread is only stopped when the daemon is stopping, so the `ThreadLocal`s are not freed-up, unless explicit `ThreadLocal.remove()` is called.
    * I am almost sure the change in Maven 3.9.0 (https://github.com/apache/maven/commit/b762fa9d5c44734b5e20eb83071e7c31821a5fad) is related here, because the `private ThreadLocal<MavenProject> currentProject` is never freed-up. There is somewhat circular dependency - `MavenProject` references `MavenSession` (indirectly, via `private ProjectBuildingRequest projectBuilderConfiguration`) and `MavenSession` references the current `MavenProject` via a thread local.
    * there is probably a fix in Maven itself which could be applied, but generally this could happen at any point in the future again (since Maven is usually just "one-shot" executed, so these kind of things are super hard to spot). I am wondering if we could do something on the Maven daemon side to avoid this?
   
   I did the following dummy experiment (I know this would need further refinement, but I just wanted to see if this approach helps or not).
   
   Instead of running the build (method call `client(socket)`) on the main "accept" thread
   ```
   private void accept() {
           try {
               while (true) {
                   try (SocketChannel socket = this.socket.accept()) {
                       client(socket);
                   }
               }
           } catch (Throwable t) {
               LOGGER.error("Error running daemon loop", t);
           }
       }
   ```
   I created a new thread for each request and then just waited for that thread to finish:
   ```
   private void accept() {
           try {
               while (true) {
                   try (SocketChannel socket = this.socket.accept()) {
                       Thread thread = new Thread(() -> client(socket));
                       thread.start();
                       thread.join();
                   }
               }
           } catch (Throwable t) {
               LOGGER.error("Error running daemon loop", t);
           }
       }
   ```
   I am well aware of the issues here (the catch is basically useless as any exception would be thrown inside that other thread). Also additional classloader configuration may be needed.
   
   In any case, this seems to "solve" the memory leak I am seeing. Once the thread finishes (stops), the `ThreadLocal`s are no longer accessible and thus are eligible for collection. Something like this would also "guard" against similar issues in the future I believe (since the thread itself is thrown away, anything it references would be thrown away as well). There may some other issues with this approach I am not seeing though.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org