ARTEMIS-5093 support configurable onMessage timeout w/closing consumer #5291
Conversation
jbertram
force-pushed
the
ARTEMIS-5093
branch
from
43f377d
to
2b6fbd1
Compare
| return config.onMessageCloseTimeout; | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
The API docs specify the value set must be greater than zero, but there isn't any validation of that, should there be?
There was a problem hiding this comment.
Hmmm...That comment was left-over from a copy & paste. I went looking through the existing code to see how validation is done for other parameters, and I'm not actually seeing any. I'm continuing my investigation.
There was a problem hiding this comment.
I did look into what it does with that configuration value and since it is passing into a CountdownLatch is seems like it will not trigger an exception but will return immediately so from the standpoint of being able to be set to zero or negative it works, although seems an absurd value to assign.
There was a problem hiding this comment.
It would seem weird to configure those if actually knowing thats what it then does, but I'd guess some might think one of those would simply mean keep waiting (e.g as is the described behaviour in the JMS spec, albeit that also originally described to effectively deadlock yourself as result hehe), especially if not reading the javadoc (should there also be actual-docs addition?).
...re-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientConsumerImpl.java
Outdated
Show resolved
Hide resolved
...s/src/test/java/org/apache/activemq/artemis/tests/integration/client/MessageHandlerTest.java
Outdated
Show resolved
Hide resolved
|
Hey sorry, I was just wondering if there was any update on this or plans to get it into a new version? Thank you so much for starting the implementation process though! |
| // 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"); |
There was a problem hiding this comment.
I'd be inclined to use a shorter timeout for the close as Dom suggests, thus reducing the test duration, but also use a longer/indefinite 'dont complete yet' guard in the handler to be sure it is always still running after/when the timeout expires and can then 'immediately complete' on request, e.g instead arrange its waiting/exit-gating on an [atomic]boolean and other latch to wait on the rest of the test then complete quickly on request rather than the current time loop.
jbertram
force-pushed
the
ARTEMIS-5093
branch
from
2b6fbd1
to
3da1cb2
Compare
There wasn't a clear place to add documentation about this so I'm relying on JavaDoc.