http-utils: fix leak in AbstractTimeoutHttpFilter #3038
Conversation
Motivation: The Single.timeout(..) family of operations are part of a class of operations will short-circuit the response based on the cancellation pathway, specifically they will return a TimeoutException down the error path. Because cancellation and the result are intrinsically racy there is currently the possibility that the result will be generated but we cannot return it to the subscriber having already given them an Exception in the error pathway. For values that are stateful this can result in a resource leak. One such leak occurs in the AbstractTimeoutHttpFilter. Modifications: Modify the AbstractTimeoutHttpFilter to keep a handle on the resource and make sure we close it if we receive a cancellation. This is a localized fix but appears to be effective. Result: One less leak.
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/AbstractTimeoutHttpFilter.java
Outdated
Show resolved
Hide resolved
| @@ -117,7 +122,9 @@ final Single<StreamingHttpResponse> withTimeout(final StreamingHttpRequest reque | |||
| final Duration timeout = timeoutForRequest.apply(request, useForTimeout); | |||
| Single<StreamingHttpResponse> response = responseFunction.apply(request); | |||
| if (null != timeout) { | |||
| final Single<StreamingHttpResponse> timeoutResponse = response.timeout(timeout, useForTimeout); | |||
| final Single<StreamingHttpResponse> timeoutResponse = response | |||
| .<StreamingHttpResponse>liftSync(CleanupSubscriber::new) | |||
There was a problem hiding this comment.
I feels like we need something similar inside JavaNetSoTimeoutHttpConnectionFilter as well. In that case, if timeout delivers onError, amb operator will do the cancel for the response and then we are in the same situation of response racing with cancel.
There was a problem hiding this comment.
How about in a followup?
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/AbstractTimeoutHttpFilter.java
Outdated
Show resolved
Hide resolved
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/AbstractTimeoutHttpFilter.java
Outdated
Show resolved
Hide resolved
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/AbstractTimeoutHttpFilter.java
Outdated
Show resolved
Hide resolved
...cetalk-http-utils/src/test/java/io/servicetalk/http/utils/AbstractTimeoutHttpFilterTest.java
Outdated
Show resolved
Hide resolved
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/AbstractTimeoutHttpFilter.java
Show resolved
Hide resolved
| try { | ||
| Object current = stateUpdater.getAndSet(this, COMPLETE); | ||
| if (current instanceof StreamingHttpResponse) { | ||
| clean((StreamingHttpResponse) current); |
There was a problem hiding this comment.
if we've propagated the StreamingHttpResponse via delegate.onSuccess() isn't it possible this will generate a duplicate subscribe? Can you add comments to clarify why this is OK? I see some comments in the test but not clear this covers all scenarios (someone may call cancel() after Subscriber.onSuccess() despite RS#2.3 and this maybe racy).
There was a problem hiding this comment.
The TL;DR is that if we get a cancel it passed through the timeout operator and 'won', therefore the message will never be propagated. I added clarifying comments and pointed to the test that ensures the behavior.
Motivation:
The Single.timeout(..) family of operations are part of a class of operations will short-circuit the response based on the cancellation pathway, specifically they will return a TimeoutException down the error path. Because cancellation and the result are intrinsically racy there is currently the possibility that the result will be generated but we cannot return it to the subscriber having already given them an Exception in the error pathway. For values that are stateful this can result in a resource leak. One such leak occurs in the AbstractTimeoutHttpFilter.
Modifications:
Modify the AbstractTimeoutHttpFilter to keep a handle on the resource and make sure we close it if we receive a cancellation. This is a localized fix but appears to be effective.
Result:
One less leak.