Add `onTimeoutException()` on RetryRule and CircuitBreakerRule by injae-kim · Pull Request #4882 · line/armeria

injae-kim · 2023-05-18T16:40:42Z

Motivation:

It would be useful to provide a way to configure retry and circuit breaker
rules to trigger than a request is timed out, before fully receving a response.

Modifications:

Add .onTimeoutException() to RetryRule and CircuitBreakerRule

Result:

You can use .onTimeoutException() on RetryRule and CircuitBreakerRule when a request is timed out before fully receiving a response.
Closes Add RetryRule.onResponseTimeout() #4876

PRs Overview

✅ Add onTimeoutException() to RetryRule and CircuitBreakerRuleto
(TODO) CancellationScheduler.finishNow(TimeoutException) on session/connection timeout to set ctx.isTimeOut=true

jrhee17

Looks good overall 👍 Left some minor questions 🙏

jrhee17 · 2023-05-22T09:20:31Z

+    public AbstractRuleBuilder onResponseTimeout() {
+        return onException((ctx, ex) -> {
+            if (ex instanceof UnprocessedRequestException) {
+                return ex.getCause() instanceof TimeoutException;


Question) Couldn't ctx.isTimedOut() still be true?

Suggested change

return ex.getCause() instanceof TimeoutException;

ex = ex.getCause();

Question) Couldn't ctx.isTimedOut() still be true?

No, it isn't. It is listed in the TODO on #4876 (comment)

(TODO) CancellationScheduler.finishNow(TimeoutException) on session/connection timeout to set ctx.isTimeOut=true

(Checked with ikhoon nim before) currently I think session/connection timeout doesn't set ctx.isTimeout=true, so I plan to fix it with next PR~!

ikhoon

Overall, looks good.

jrhee17

Left a minor question but looks good to me 👍 Thanks @injae-kim 🙇 👍 🚀

jrhee17 · 2023-05-23T14:53:26Z

+            return ex instanceof UnprocessedRequestException &&
+                   ex.getCause() instanceof TimeoutException;


~~Question since I'm not sure either) Do we also want to check for cases where a TimeoutException is thrown directly from a upstream decorator? e.g. ConcurrencyLimitTimeoutException~~

Suggested change

return ex instanceof UnprocessedRequestException &&

ex.getCause() instanceof TimeoutException;

return ex instanceof TimeoutException ||

ex instanceof UnprocessedRequestException &&

ex.getCause() instanceof TimeoutException;

Ignore this comment 😅

Shouldn't we do this? If users manually complete the response exceptionally with a ResponseTimeoutException from a decorator, it wouldn't work.
This might also confuse users that onTimeoutException() is invoked only when the exception is wrapped with the UnprocessedRequestException even though it isn't.

If users manually complete the response exceptionally with a ResponseTimeoutException from a decorator, it wouldn't work.

oh~ this case can happen. @ikhoon nim, WDYT? (how about check ex instanceof TimeoutException too?)

If users manually complete the response exceptionally with a ResponseTimeoutException from a decorator, it wouldn't work.

Hmm... ResponseTimeoutException is a special exception to cancel a response based on the timeout stored in ctx.responseTimeoutMillis(). If a user raises ResponseTimeoutException, it should be assumed as a normal exception but neither a timeout exception or a cancellation exception.

This might also confuse users that onTimeoutException() is invoked only when the exception is wrapped with the UnprocessedRequestException even though it isn't.

If we update the contract of ctx.isTimedOut() properly, I don't see users get confused of the behavior. They may not judge the behavior by the implementation.

Anyway, it is good to add ex instanceof TimeoutException in this PR because ctx.isTimedOut() doesn't cover WriteTimeoutException and ConcurrencyLimitTimeoutException even though we will handle them in the following PR.
Let's make this PR solid itself.

Anyway, it is good to add ex instanceof TimeoutException in this PR

✅ updated!

minwoox

Looks great all in all. 👍
Left just a small opinion.

minwoox · 2023-05-24T09:46:12Z

+            return ex instanceof UnprocessedRequestException &&
+                   ex.getCause() instanceof TimeoutException;


Shouldn't we do this? If users manually complete the response exceptionally with a ResponseTimeoutException from a decorator, it wouldn't work.
This might also confuse users that onTimeoutException() is invoked only when the exception is wrapped with the UnprocessedRequestException even though it isn't.

ikhoon

Thanks, @injae-kim! 🙇‍♂️

minwoox

Looks good all in all, thanks a lot! @injae-kim
BTW, could you revise the title and the description of this PR? 😄

injae-kim · 2023-06-02T07:28:19Z

oh~ I updated PR title&desc, onResponseTimeout -> onTimeoutException!

trustin

Lovely! Thanks, @injae-kim 🙇

…4882) Motivation: It would be useful to provide a way to configure retry and circuit breaker rules to trigger than a request is timed out, before fully receving a response. Modifications: - Add `.onTimeoutException()` to `RetryRule` and `CircuitBreakerRule` Result: - You can use `.onTimeoutException()` on RetryRule and CircuitBreakerRule when a request is timed out before fully receiving a response. - Closes line#4876

Add onResponseTimeout() on RetryRule and CircuitBreakerRule

f09e6b5

injae-kim requested review from ikhoon, jrhee17, minwoox and trustin as code owners May 18, 2023 16:40

injae-kim mentioned this pull request May 18, 2023

Add RetryRule.onResponseTimeout() #4876

Closed

jrhee17 reviewed May 22, 2023

View reviewed changes

jrhee17 added the new feature label May 22, 2023

jrhee17 added this to the 1.24.0 milestone May 22, 2023

ikhoon reviewed May 22, 2023

View reviewed changes

Comment thread core/src/main/java/com/linecorp/armeria/client/AbstractRuleBuilder.java

injae-kim added 2 commits May 22, 2023 21:37

Rename to onTimeoutException()

3d654e6

Address comment on onTimeoutException()

0714dc9

injae-kim requested review from ikhoon and jrhee17 May 23, 2023 04:50

ikhoon reviewed May 23, 2023

View reviewed changes

jrhee17 approved these changes May 23, 2023

View reviewed changes

Address minor comments

2e8a56f

minwoox reviewed May 24, 2023

View reviewed changes

Check TimeoutException too

3f31232

ikhoon approved these changes May 26, 2023

View reviewed changes

minwoox approved these changes Jun 2, 2023

View reviewed changes

injae-kim changed the title ~~Add onResponseTimeout() on RetryRule and CircuitBreakerRule~~ Add onTimeoutException() on RetryRule and CircuitBreakerRule Jun 2, 2023

trustin approved these changes Jun 9, 2023

View reviewed changes

Merge branch 'main' into add-onResponseTimeout-on-retry-and-circuit-rule

b7043f9

trustin mentioned this pull request Jun 9, 2023

A DnsTimeoutException should also set true to ctx.isTimedOut(). #4935

Closed

trustin merged commit 9b831fb into line:main Jun 9, 2023

	return ex.getCause() instanceof TimeoutException;
	ex = ex.getCause();

		return ex instanceof UnprocessedRequestException &&
		ex.getCause() instanceof TimeoutException;

Conversation

injae-kim commented May 18, 2023 • edited by trustin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrhee17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrhee17 May 22, 2023

Choose a reason for hiding this comment

Uh oh!

ikhoon May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

injae-kim May 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrhee17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrhee17 May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minwoox May 24, 2023

Choose a reason for hiding this comment

Uh oh!

injae-kim May 25, 2023

Choose a reason for hiding this comment

Uh oh!

ikhoon May 26, 2023

Choose a reason for hiding this comment

Uh oh!

injae-kim May 26, 2023

Choose a reason for hiding this comment

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

minwoox May 24, 2023

Choose a reason for hiding this comment

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

injae-kim commented Jun 2, 2023

Uh oh!

trustin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

injae-kim commented May 18, 2023 •

edited by trustin

Loading

ikhoon May 22, 2023 •

edited

Loading

jrhee17 May 23, 2023 •

edited

Loading