gemmellr commented on code in PR #5291:
URL: https://github.com/apache/activemq-artemis/pull/5291#discussion_r1856738639
##########
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:
The existing latch usage at the start is fine. The comments were all around
the end/exit of the handler and the assert around 'the duration' between start
and end on the test thread (point after the latch wait returns, until after the
close returns). Those bits are entirely reliant on nice scheduling of the test
thread and the handler thread within the 600ms window the test allows for
success to occur before it fails. I'm saying add a mechanism, e.g boolean /
other latch, so that it isn't susceptible to such failure due to scheduling.
All I'm saying is remove the brittle time based windowing, and letting the
handler thread stick around for 800ms even when you might be done with it
already, and instead just ensure the handler thread stays in play until the
point you want want it to exit...then you can assert, with no brittle
scheduling effects possible, that the close completed in >timeout and
<handler-exit-point, and let the handler thread complete. Same asserts, no
brittle mechanism that could sporadically fail on the whims of resource
availability/usage in the 600ms period, and no need to leave the handler thread
looping in play for some point up to 800ms later than the 'actual test' is done.
--
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
