Fix duration comparison bug in timeout filters and reduce code duplication #1491
Conversation
servicetalk-utils-internal/src/main/java/io/servicetalk/utils/internal/DurationUtils.java
Show resolved
Hide resolved
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/AbstractTimeoutHttpFilter.java
Show resolved
Hide resolved
…ation Motivation: `Duration#compareTo` is hard to understand, the result of comparison depends on arguments order. This lead to a bug in `TimeoutHttpRequesterFilter` and `TimeoutHttpServiceFilter` which always fails payload body publisher with `TimeoutException`. Use a shared utility to make this consistent across all modules. Also, `TimeoutHttpRequesterFilter` and `TimeoutHttpServiceFilter` logic is identical, we can reduce duplication and share to logic. Modifications: - Introduce a shared `DurationUtils` class for common operations with `Duration`; - Use these utility to reduce mistakes when we check that the passed `Duration` must be positive; - Introduce `AbstractTimeoutHttpFilterTest` to share timeout logic between `TimeoutHttpRequesterFilter` and `TimeoutHttpServiceFilter`; - Add tests to verify behavior correctness for both timeout filters; - Adjust how the timeout filters are created in `ResponseTimeoutTest` to avoid NPE; - Minor improvements to make javadoc consistent between `TimeoutHttpRequesterFilter` and `TimeoutHttpServiceFilter`; Result: 1. `TimeoutHttpRequesterFilter` and `TimeoutHttpServiceFilter` do not fail payload body publisher before the actual timeout expires. 2. No code duplication between timeout filters. 3. Shared utility to consistently validate `Duration` in all modules.
idelpivnitskiy
force-pushed
the
fix-timeout-filters
branch
from
eba119d to
5f4b776
Compare
| @@ -168,8 +168,8 @@ private void requestCancellationResetsStreamButNotParentConnection(StreamingHttp | |||
| // ignore | |||
| } | |||
| }) | |||
| .expectError() | |||
| // .thenCancel() | |||
| // FIXME: use thenCancel() after await() instead of cancelling from inside then(...) + expectError() | |||
There was a problem hiding this comment.
can you create an issue and link it here?
|
|
||
| return Single.defer(() -> { | ||
| final Duration timeout = timeoutForRequest.apply(request); | ||
| if (null != timeout && !isPositive(timeout)) { |
There was a problem hiding this comment.
do we need todo the pre-checks? ideally the timeout operators handle this case and we can simplify here. we would pay a bit more cost to construct the operators, but if the operators don't handle this we would have to apply these pre-checks everywhere we use the operators (not ideal).
There was a problem hiding this comment.
currently - yes. it's a feature of timeoutForRequest. if it returns null, it means no timeout. If it returns ZERO or negative => users want timeout right away.
I will keep it as-is for now. If we will move this logic to timeout operators in a follow-up, we can simplify here. For now, limiting the scope to the filters only
There was a problem hiding this comment.
The operators do handle it, they explicitly accept negative duration which is expected to occur as a result of calculated timeouts. The appearance here of pre-checks was as a result of earlier review feedback to simplify the flow.
There was a problem hiding this comment.
null is is intended to impact control flow (e.g. null means "no timeout") so it is expected to check for null before applying timeout operators (e.g. doesn't make sense to apply a timeout if there is no timeout) . however if there is a timeout value the timeout operators should be able to handle it.
There was a problem hiding this comment.
I would consider that as a follow-up only if the operator is improved. Right now operator will always schedule a task, which creates more overhead (also does a thread hop) compare to this check.
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/AbstractTimeoutHttpFilter.java
Outdated
Show resolved
Hide resolved
Motivation:
Duration#compareTois hard to understand, the result of comparisondepends on the order of arguments. This lead to a bug in
TimeoutHttpRequesterFilterandTimeoutHttpServiceFilterwhich alwaysfails payload body publisher with
TimeoutException. Use a sharedutility to make
isPositivecheck forDurationconsistent across all modules.Also,
TimeoutHttpRequesterFilterandTimeoutHttpServiceFilterlogicis identical, we can reduce duplication and share the logic.
Modifications:
DurationUtilsclass for common operations withDuration;Durationmust be positive;AbstractTimeoutHttpFilterTestto share timeout logicbetween
TimeoutHttpRequesterFilterandTimeoutHttpServiceFilter;ResponseTimeoutTestto avoid NPE;
TimeoutHttpRequesterFilterandTimeoutHttpServiceFilter;Result:
TimeoutHttpRequesterFilterandTimeoutHttpServiceFilterdo notfail payload body publisher before the actual timeout expires.
Durationin all modules.