HttpTimeoutFilter include original timeout on message body timeout #1494
Conversation
Motivation: AbstractTimeoutHttpFilter can now apply a timeout on the metadata and the message body level. If the message body level timeout triggers the interval is taken as the user will see a value which is different than the originally supplied timeout duration. Modifications: - Add a MappedTimeoutException that can wrap a TimeoutException and allows for specifying the original timeout value - Remove pre-check conditions on timeout duration which is enforced by the timeout operators. Result: HttpTimeoutFilter exception includes the original timeout duration.
| @@ -73,10 +71,6 @@ public final HttpExecutionStrategy influenceStrategy(final HttpExecutionStrategy | |||
|
|
|||
| return Single.defer(() -> { | |||
| final Duration timeout = timeoutForRequest.apply(request); | |||
| if (null != timeout && !isPositive(timeout)) { | |||
| return Single.failed(new TimeoutException("non-positive timeout of " + timeout.toMillis() + "ms")); | |||
There was a problem hiding this comment.
doing the short-circuit evaluation also bypasses the timeoutExecutor. which maybe desirable in some circumstances but also changes threading/control flow. ideally we can consolidate the behavior (e.g. should we notify outside a timeoutExecutor thread) in the operators to ensure consistency.
There was a problem hiding this comment.
if the timeout is negative we will have a thread hop between GLOBAL_SINGLE_THREADED_SCHEDULED_EXECUTOR and offloadExecutor. Sounds expensive if you know that the result will be a TimeoutException anyway. So, I'm not sure this change is worth it unless the operator handles negative values and returns TimeoutException without touching an executor.
I understood what you are saying about "changes threading/control flow". But looks like we have much bigger issues related to GLOBAL_SINGLE_THREADED_SCHEDULED_EXECUTOR. Consider the following two scenarios:
- request is executed on IO thread, but the filter is configured using a global executor. the timeout
onErrorwill be propagated on a thread from the global executor, breaking the asynchronous expectations of the client. This is likely a user's mistake (misconfiguration), but it's hard to catch it because users will probably expect that only the scheduler will be taken from thetimeoutExecutorbut won't expect thattimeoutExecutorwill be used foronErrorpropagation. - request is executed on a thread from the global executor (default offloading model), but the timeout filter is configured without a custom
timeoutExecutor. As a result, we will useimmediate()executor. It will schedule a timer, when time expiresSingleThreadedScheduleroffloads toimmediate(). As the result,onErroris propagated on aservicetalk-global-schedulerthread 🤯. If user code blocks, it negatively affects every other task on aGLOBAL_SINGLE_THREADED_SCHEDULED_EXECUTOR😢
My personal expectation as a user: if I provide a timerExecutor it should be used only to schedule a timeout. The TimeoutExeception should be delivered on the same executor I'm currently using.
There was a problem hiding this comment.
Agreed we should be more explicit about offloading and executor selection w.r.t to timeouts (especially when we apply timeout filters in the background). I'll create an issue to track that. However this PR doesn't change existing behavior and just ensures we always delegate to the timeout operator to make the decision. When we do revisit this topic all we have todo is make sure we supply the right executor, and the executor can determine if it needs to schedule a task if the duration is <=0 or invoke the runnable synchronously.
Also note that @bondolo is re-working the offloading which will have impacts here.
There was a problem hiding this comment.
Agreed, let's make the behavior consistent with 0.38.1 and we can work on further improvements.
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/AbstractTimeoutHttpFilter.java
Show resolved
Hide resolved
| @@ -73,10 +71,6 @@ public final HttpExecutionStrategy influenceStrategy(final HttpExecutionStrategy | |||
|
|
|||
| return Single.defer(() -> { | |||
| final Duration timeout = timeoutForRequest.apply(request); | |||
| if (null != timeout && !isPositive(timeout)) { | |||
| return Single.failed(new TimeoutException("non-positive timeout of " + timeout.toMillis() + "ms")); | |||
There was a problem hiding this comment.
if the timeout is negative we will have a thread hop between GLOBAL_SINGLE_THREADED_SCHEDULED_EXECUTOR and offloadExecutor. Sounds expensive if you know that the result will be a TimeoutException anyway. So, I'm not sure this change is worth it unless the operator handles negative values and returns TimeoutException without touching an executor.
I understood what you are saying about "changes threading/control flow". But looks like we have much bigger issues related to GLOBAL_SINGLE_THREADED_SCHEDULED_EXECUTOR. Consider the following two scenarios:
- request is executed on IO thread, but the filter is configured using a global executor. the timeout
onErrorwill be propagated on a thread from the global executor, breaking the asynchronous expectations of the client. This is likely a user's mistake (misconfiguration), but it's hard to catch it because users will probably expect that only the scheduler will be taken from thetimeoutExecutorbut won't expect thattimeoutExecutorwill be used foronErrorpropagation. - request is executed on a thread from the global executor (default offloading model), but the timeout filter is configured without a custom
timeoutExecutor. As a result, we will useimmediate()executor. It will schedule a timer, when time expiresSingleThreadedScheduleroffloads toimmediate(). As the result,onErroris propagated on aservicetalk-global-schedulerthread 🤯. If user code blocks, it negatively affects every other task on aGLOBAL_SINGLE_THREADED_SCHEDULED_EXECUTOR😢
My personal expectation as a user: if I provide a timerExecutor it should be used only to schedule a timeout. The TimeoutExeception should be delivered on the same executor I'm currently using.
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/AbstractTimeoutHttpFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I am unsure on the short-circuiting issue. I would like to think that it should be possible for this kind of short circuiting to safely take place as long as it can be done safely. Having to invoke the entire timeout machinery so that the threading/control flow would be consistent seems too sensitive.
| @@ -73,10 +71,6 @@ public final HttpExecutionStrategy influenceStrategy(final HttpExecutionStrategy | |||
|
|
|||
| return Single.defer(() -> { | |||
| final Duration timeout = timeoutForRequest.apply(request); | |||
| if (null != timeout && !isPositive(timeout)) { | |||
| return Single.failed(new TimeoutException("non-positive timeout of " + timeout.toMillis() + "ms")); | |||
There was a problem hiding this comment.
Agreed, let's make the behavior consistent with 0.38.1 and we can work on further improvements.
Motivation:
AbstractTimeoutHttpFilter can now apply a timeout on the metadata and
the message body level. If the message body level timeout triggers the
interval is taken as the user will see a value which is different than
the originally supplied timeout duration.
Modifications:
allows for specifying the original timeout value
the timeout operators.
Result:
HttpTimeoutFilter exception includes the original timeout duration.