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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
