[GitHub] [maven-mvnd] psiroky commented on a diff in pull request #799: Run client connection handler inside new thread, fixes #798

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

psiroky commented on code in PR #799:
URL: https://github.com/apache/maven-mvnd/pull/799#discussion_r1126459819


##########
daemon/src/main/java/org/mvndaemon/mvnd/daemon/Server.java:
##########
@@ -233,7 +233,16 @@ private void accept() {
         try {
             while (true) {
                 try (SocketChannel socket = this.socket.accept()) {
-                    client(socket);
+                    try {
+                        // execute the client connection handling inside a new thread to guard against possible
+                        // ThreadLocal memory leaks
+                        // see https://github.com/apache/maven-mvnd/issues/798 for more details
+                        Thread handler = new Thread(() -> client(socket));
+                        handler.start();
+                        handler.join();
+                    } catch (Throwable t) {
+                        LOGGER.error("Error handling a client connection", t);
+                    }

Review Comment:
   I think this technically changes the semantics a bit. Before, if the `client(socket)` threw an exception, then the whole `while` loop would end, which is not the case anymore. If there is a bug somewhere, then I guess the previous version of the code could fail fast (leaving the while loop), while the new would simply be waiting for another connection. Not sure how big of problem this is.



-- 
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