Support for timeout propagation in async requests #617
Conversation
| * @param timeout the time to wait for a response | ||
| * @return a Future for the response, which may be cancelled on error or timed out | ||
| */ | ||
| CompletableFuture<Message> requestWithTimeout(Message message, Duration timeout); |
There was a problem hiding this comment.
For parity, I think we'd also want:
CompletableFuture<Message> requestWithTimeout(String subject, byte[] body, Duration timeout);wdyt @scottf ?
| @@ -239,6 +239,39 @@ public void testRequireCleanupOnTimeoutNoNoResponders() throws IOException, Inte | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| public void testRequireCleanupOnTimeoutCleanCompletable() throws IOException, InterruptedException { | |||
There was a problem hiding this comment.
I'd like to see some tests that also succeed, and then fail with a timeout. This one does test failure w/ cleanup - but would like to see that the right exception is thrown.
There was a problem hiding this comment.
Hi! For a "succeed and then fail" what do you mean exactly? I see that the timeout of the NatsRequestCompletableFuture is considered only upon cleanup.
I can add an additional test that triggers the timeout exception, coming from the CompletableFuture.
Something like:
assertThrows(TimeoutException.class, () -> nc.requestWithTimeout(nm, Duration.ofMillis(cleanupInterval)).get(100, TimeUnit.MILLISECONDS));
Thanks for your reply
There was a problem hiding this comment.
so this this is request/reply, the timeout is suggesting how long to wait for a response. Test actually getting a response back and also not getting a response back. Although No responders may come into play. What do other tests for the default timeout do?
There was a problem hiding this comment.
added more tests and api update. thx for the clarification
|
Thanks for contributing! I offered a few suggestions but will defer to @scottf. |
|
I see 5 failures in the build, after the pushed merge commit:
-> Theses are not linked to the change (and the new exposed API), let me know if I need to take some actions in order to have this merged. Thanks! |
|
|
Using admin merge because GHA/coveralls isn't reporting after hours of waiting. |
Give the possibility to clients to specify a timeout while performing async requests.
More details in #616