[PR] Fixed race condition in TServerUtils exposed by TServerUtilsTest [accumulo]

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

dlmarion opened a new pull request, #4413:
URL: https://github.com/apache/accumulo/pull/4413

   There are several tests in TServerUtilsTest that have the form:
   
   ```
       TServer server = null;
       try {
         ServerAddress address = startServer();
         server = address.getServer();
       } finally {
         if (server != null) {
           server.stop();
         }
       }
   ```
   
   The TServerUtilsTest.startServer method calls TServerUtils.startServer which ends up creating a Thread that calls TServer.serve(). When TServer is a TThreadPoolServer, the serve method calls preServe first, which sets the internal boolean variable `stopped_` to false, and then calls execute which will loop while `stopped_` is false.
   
   In the case where the Thread created by TServerUtils.startServer is not started right away, then it's possible that the test method will call stop (setting `stopped_` to true) before the TServer.serve method calls preServe (setting `stopped_` back to false) resulting in the Thread being in an endless loop.
   
   This can be seen by running TServerTestUtils, where the output contains many lines like:
   
   ```
   [server.TThreadPoolServer] WARN : Transport error occurred during acceptance of message
   org.apache.thrift.transport.TTransportException: No underlying server socket.
           at org.apache.thrift.transport.TServerSocket.accept(TServerSocket.java:113) ~[libthrift-0.17.0.jar:0.17.0]
           at org.apache.thrift.transport.TServerSocket.accept(TServerSocket.java:31) ~[libthrift-0.17.0.jar:0.17.0]
           at org.apache.thrift.server.TThreadPoolServer.execute(TThreadPoolServer.java:162) ~[libthrift-0.17.0.jar:0.17.0]
           at org.apache.thrift.server.TThreadPoolServer.serve(TThreadPoolServer.java:148) ~[libthrift-0.17.0.jar:0.17.0]
           at org.apache.accumulo.server.rpc.TServerUtils.lambda$startTServer$9(TServerUtils.java:654) ~[classes/:?]
           at org.apache.accumulo.core.trace.TraceWrappedRunnable.run(TraceWrappedRunnable.java:52) [accumulo-core-2.1.3-SNAPSHOT.jar:2.1.3-SNAPSHOT]
           at java.base/java.lang.Thread.run(Thread.java:840) [?:?]
   ```
   
   Because the surefire configuration reuses JVM forks these Threads persist for the duration of the unit tests in server base and pollute every test output file after TServerUtilsTest is executed. The surefire forkCount is set to `1C`, so the volume of output in the logs is dependent on the number of forks.


-- 
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: notifications-unsubscribe@accumulo.apache.org

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

Re: [PR] Fixed race condition in TServerUtils exposed by TServerUtilsTest [accumulo]

["keith-turner (via GitHub)" <gi...@apache.org>]

keith-turner commented on code in PR #4413:
URL: https://github.com/apache/accumulo/pull/4413#discussion_r1534879056


##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
@@ -657,6 +658,12 @@ private static ServerAddress startTServer(ThriftServerType serverType, TimedProc
       }
     }).start();
 
+    while (!finalServer.isServing()) {
+      // Wait for the thread to start and for the TServer to start
+      // serving events
+      UtilWaitThread.sleep(10);
+    }

Review Comment:
   Could add a defensive check for future code changes.
   
   ```suggestion
          Preconditions.checkState(!finalServer.getShouldStop());
       }
   ```



-- 
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: notifications-unsubscribe@accumulo.apache.org

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

Re: [PR] Fixed race condition in TServerUtils exposed by TServerUtilsTest [accumulo]

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

dlmarion merged PR #4413:
URL: https://github.com/apache/accumulo/pull/4413


-- 
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: notifications-unsubscribe@accumulo.apache.org

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