gemmellr commented on code in PR #5291:
URL: https://github.com/apache/activemq-artemis/pull/5291#discussion_r1856960550
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/client/MessageHandlerTest.java:
##########
@@ -117,6 +118,42 @@ public void onMessage(final ClientMessage message) {
session.close();
}
+ @Test
+ public void testMessageHandlerCloseTimeout() throws Exception {
+ // create Netty acceptor so client can use new onMessageCloseTimeout URL
parameter
+ server.getRemotingService().createAcceptor("netty",
"tcp://127.0.0.1:61616").start();
+ final int timeout = 1000;
+ locator =
ActiveMQClient.createServerLocator("tcp://127.0.0.1:61616?onMessageCloseTimeout="
+ timeout);
+ sf = createSessionFactory(locator);
+ ClientSession session = addClientSession(sf.createSession(false, true,
true));
+ session.createQueue(QueueConfiguration.of(QUEUE).setDurable(false));
+ ClientProducer producer = session.createProducer(QUEUE);
+ producer.send(createTextMessage(session, "m"));
+
+ ClientConsumer consumer = session.createConsumer(QUEUE, null, false);
+
+ session.start();
+
+ CountDownLatch latch = new CountDownLatch(1);
+ consumer.setMessageHandler(message -> {
+ latch.countDown();
+ // don't just Thread.sleep() here because it will be interrupted on
ClientConsumer.close()
+ long start = System.currentTimeMillis();
+ while (System.currentTimeMillis() - start < 2000) {
+ try {
+ Thread.sleep(100);
+ } catch (InterruptedException e) {
+ // ignore
+ }
+ }
+ });
+ latch.await();
+ long start = System.currentTimeMillis();
+ consumer.close();
+ long end = System.currentTimeMillis();
+ assertTrue( (end - start >= timeout) && (end - start <= 2000), "Closing
consumer took " + (end - start) + "ms");
Review Comment:
You stopped the thread hanging around much longer than the checks, which is
good...however the checks still remain as brittle to scheduling as they were
before the change.
The main point is surely that it did not exit during the close timeout, and
that the close takes at least the timeout. So you control the handler thread
cant exit while those checks are happening, but then exits immediately as soon
as they have completed, once you allow it to. The 800ms magic window doesnt
need to be there.
Is it likely to take that long? No. Though could it happen now and then, on
e.g a slow CI env with low thread count and shared resources? Yes, when someone
then needs to go look at the sporadic failure. Or it adds to the many that have
sporadically failed and encouraged people not to look.
If you want to use a timeout in the handler that fair enough, but I'd make
it a lot longer so it can never reasonably happen unless the test has hung
elsewhere.
It also needs add a try-finally to ensure the gating boolean is tripped to
have it exit immediately even if the asserts somehow failed.
--
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: gitbox-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
