GH-2478 Handle conversion exception in AsyncRabbitTemplate #2932
Conversation
|
@BenEfrati Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@BenEfrati Thank you for signing the Contributor License Agreement! |
There was a problem hiding this comment.
Please, run ./gradlew check locally before pushing changes to PR.
Right now it even does not compile:
/home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/AsyncRabbitTemplate.java:614: error: cannot find symbol
} catch (MessageConversionException e) {
^
symbol: class MessageConversionException
location: class AsyncRabbitTemplate
|
Hi, |
…late Previously, conversion errors in AsyncRabbitTemplate lead to AmqpReplyTimeoutException
spring-rabbit/src/test/java/org/springframework/amqp/rabbit/AsyncRabbitTemplateTests.java
Show resolved
Hide resolved
| cdl.countDown(); | ||
| }); | ||
| assertThat(cdl.await(10, TimeUnit.SECONDS)).isTrue(); | ||
| assertThat(replyFuture).isCompletedExceptionally(); |
There was a problem hiding this comment.
May we also verify what exactly exception we got here?
| final CountDownLatch cdl = new CountDownLatch(1); | ||
| final AtomicReference<Object> resultRef = new AtomicReference<>(); | ||
| replyFuture.whenComplete((result, ex) -> { | ||
| resultRef.set(result); | ||
| cdl.countDown(); | ||
| }); | ||
| assertThat(cdl.await(10, TimeUnit.SECONDS)).isTrue(); | ||
| assertThat(replyFuture).isCompletedExceptionally(); |
There was a problem hiding this comment.
| final CountDownLatch cdl = new CountDownLatch(1); | |
| final AtomicReference<Object> resultRef = new AtomicReference<>(); | |
| replyFuture.whenComplete((result, ex) -> { | |
| resultRef.set(result); | |
| cdl.countDown(); | |
| }); | |
| assertThat(cdl.await(10, TimeUnit.SECONDS)).isTrue(); | |
| assertThat(replyFuture).isCompletedExceptionally(); | |
| assertThat(replyFuture).failsWithin(Duration.ofSeconds(10)) | |
| .withThrowableThat() | |
| .withCauseInstanceOf(MessageConversionException.class); |
Something like this?
There was a problem hiding this comment.
Sounds good!
But does not look like it would be a good candidate to commit such a suggestion: too many broken indents.
There was a problem hiding this comment.
Or never mind. I like it.
You have changed the logic, but at the same it is covered with that failsWithin() 😄
Committing and merging.
Thank you!
| }); | ||
| assertThat(cdl.await(10, TimeUnit.SECONDS)).isTrue(); | ||
| assertThat(replyFuture).isCompletedExceptionally(); | ||
| assertThat(resultRef.get()).isNull(); |
There was a problem hiding this comment.
But this line has to be removed then 😄
There was a problem hiding this comment.
sure :)
Do you want me to align other tests to use failsWithin
void testReturn() {
...
assertThat(future)
.as("Expected exception")
.failsWithin(Duration.ofSeconds(10))
.withThrowableThat()
.withCauseInstanceOf(AmqpMessageReturnedException.class)
.extracting("routingKey")
.isEqualTo(this.requests.getName() + "x");
instead
try {
future.get(10, TimeUnit.SECONDS);
fail("Expected exception");
}
catch (ExecutionException e) {
assertThat(e.getCause()).isInstanceOf(AmqpMessageReturnedException.class);
assertThat(((AmqpMessageReturnedException) e.getCause()).getRoutingKey()).isEqualTo(this.requests.getName() + "x");
}
There was a problem hiding this comment.
I would appreciate.
Thanks.
Those try..catch in tests is on my TODO list in favor of assertThat() when I touch these classes.
Although still would be with some code.
But now I have learned about failsWithin() which looks much cleaner. 😄
|
thank you for contribution; looking forward for more! |
Previously, conversion errors in AsyncRabbitTemplate lead to AmqpReplyTimeoutException
Fixes #2478
#2478 (comment)