`ProxyConnectConnectionFactoryFilter` leaks connection in case of errors #1002

idelpivnitskiy · 2020-04-04T00:34:52Z

Motivation:

In case of any error or cancellation, ProxyConnectConnectionFactoryFilter
does not close the connection to the proxy.

Modifications:

Add tests to verify different failure scenarios for
ProxyConnectConnectionFactoryFilter;
Close connection when CONNECT request or SSL handshake failures;
Do not drain the response payload body in case of error, because the
connection will be closed anyway;

Result:

ProxyConnectConnectionFactoryFilter does not leak connections.

Motivation: In case of any error or cancellation, `ProxyConnectConnectionFactoryFilter` does not close the connection to the proxy. Modifications: - Add tests to verify different failure scenarios for `ProxyConnectConnectionFactoryFilter`; - Close connection when `CONNECT` request or SSL handshake failures or if a `Single` is cancelled; Result: `ProxyConnectConnectionFactoryFilter` does not leak connections.

...-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java

idelpivnitskiy · 2020-04-07T05:10:19Z

...-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java

+                            // Close recently created connection in case of any error while it connects to the proxy
+                            // or cancellation:
+                            .recoverWith(t -> c.closeAsync().concat(failed(t)))
+                            .whenCancel(() -> c.closeAsync().subscribe());


we really do not know if the cancel has come due to the operation was canceled by the user or due to some operators sending a cancel for the previous source when they move on to the next source (eg: concat()).

Can any operator cancel after success? IIUC they cancel the previous source only for non-success/non-complete cases.

LMK if I need to revert whenFinally here to prevent closure on cancel after success.

Can any operator cancel after success?

Yes they do, consider this connFactory.newConnection().concat(executor.timer(1, MILLISECONDS) (eg: to add a delay to respond to connect)

concat() uses SequentialCancellable which cancels the old Cancellable when the new Cancellable is received, which in this case will be after the successful completion of connFactory.newConnection().

More generally, we should not assume anywhere that cancel is only received before success() as Cancellable and Subscriber code paths are concurrent.

Looking at SequentialCancellable and TBH don't see where it cancels the old Cancellable. When the new Cancellable is received it may close the new one immediately if the oldVal was already canceled via SequentialCancellable#cancel().

More generally, we should not assume anywhere that cancel is only received before success() as Cancellable and Subscriber code paths are concurrent.

Agreed. I just thought that it doesn't matter when proxy filter sees cancel: before or after onSuccess we should close the connection if we saw that someone is not interested in the result anymore.

Btw, after #1005, should it be afterCancel or afterFinally?

whenCancel will unconditionally execute the callback when cancel is called regardless if the connection has been delivered downstream. If we have already delivered the connection we shouldn't later close it (regardless if someone cancels or not). In addition to this being the expected control flow, the RS spec has some rules which discuss cancel being a no-op after a terminal signal is delivered [1][2].

afterFinally(SingleTerminalSignalConsumer<T> doFinally) happens to enforce "only a single callback will be executed" but may still result in invoking the onCancel() call back and also calling the downstream Subscriber#onSuccess(...) for the following reasons:

Subscription can be invoked on a different thread

Data/terminal signals may still be delivered after cancel [3]

So afterFinally is an improvement over afterCancel, but still isn't ideal because we may deliver a closed object (and/or invoke closeAsync() concurrently).

[1] https://github.com/reactive-streams/reactive-streams-jvm/blob/v1.0.3/README.md#1.6

If a Publisher signals either onError or onComplete on a Subscriber, that Subscriber’s Subscription MUST be considered cancelled.

[2] https://github.com/reactive-streams/reactive-streams-jvm/blob/v1.0.3/README.md#3.7

After the Subscription is cancelled, additional Subscription.cancel() MUST be NOPs.

[3] https://github.com/reactive-streams/reactive-streams-jvm/blob/v1.0.3/README.md#2.8

A Subscriber MUST be prepared to receive one or more onNext signals after having called Subscription.cancel() if there are still requested elements pending [see 3.12]. Subscription.cancel() does not guarantee to perform the underlying cleaning operations immediately.

TBH don't see where it cancels the old Cancellable.

Aah, you are correct. I misread under an older assumption that we cancel() the previous Cancellable.

Anyways, for other reasons me and Scott mention, unconditional close() upon cancel() isn't correct.

Updated to use ~~whenFinally d2bf22f~~ afterFinally cfd0117

So afterFinally is an improvement over afterCancel, but still isn't ideal because we may deliver a closed object (and/or invoke closeAsync() concurrently).

Ok ya this seems to be a problem. Can we remove the close-on-cancel part for now?

Connection lifetime is anyways a problem in such situation out of the context of this filter as mentioned in #1002 (comment).

Lets fix the obvious issue of leaking connection for non-200 responses and then handle lifecycle on cancel/early termination later.

Removed in 57e470b and created #1010.

idelpivnitskiy · 2020-04-07T05:12:33Z

...-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java

-                        }
-                    });
+    private Single<C> handleConnectResponse(final C connection, final StreamingHttpResponse response) {
+        try {


Moved this logic to the different method because nested try blocks make code indentation awful.

Scottmitch · 2020-04-07T23:05:01Z

...-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java

+                            // Close recently created connection in case of any error while it connects to the proxy
+                            // or cancellation:
+                            .recoverWith(t -> c.closeAsync().concat(failed(t)))
+                            .whenCancel(() -> c.closeAsync().subscribe());


whenCancel will unconditionally execute the callback when cancel is called regardless if the connection has been delivered downstream. If we have already delivered the connection we shouldn't later close it (regardless if someone cancels or not). In addition to this being the expected control flow, the RS spec has some rules which discuss cancel being a no-op after a terminal signal is delivered [1][2].

afterFinally(SingleTerminalSignalConsumer<T> doFinally) happens to enforce "only a single callback will be executed" but may still result in invoking the onCancel() call back and also calling the downstream Subscriber#onSuccess(...) for the following reasons:

Subscription can be invoked on a different thread

Data/terminal signals may still be delivered after cancel [3]

So afterFinally is an improvement over afterCancel, but still isn't ideal because we may deliver a closed object (and/or invoke closeAsync() concurrently).

[1] https://github.com/reactive-streams/reactive-streams-jvm/blob/v1.0.3/README.md#1.6

If a Publisher signals either onError or onComplete on a Subscriber, that Subscriber’s Subscription MUST be considered cancelled.

[2] https://github.com/reactive-streams/reactive-streams-jvm/blob/v1.0.3/README.md#3.7

After the Subscription is cancelled, additional Subscription.cancel() MUST be NOPs.

[3] https://github.com/reactive-streams/reactive-streams-jvm/blob/v1.0.3/README.md#2.8

A Subscriber MUST be prepared to receive one or more onNext signals after having called Subscription.cancel() if there are still requested elements pending [see 3.12]. Subscription.cancel() does not guarantee to perform the underlying cleaning operations immediately.

...-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java

…ction

idelpivnitskiy · 2020-04-08T05:18:51Z

Build failed because of an issue with docker.

@servicetalk-bot test this please

idelpivnitskiy · 2020-04-08T22:27:36Z

jdk8 build failed with timeout

@servicetalk-bot test this please

NiteshKant

LGTM after these comments

...-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java

NiteshKant · 2020-04-08T22:57:14Z

...-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java

+                            .flatMap(response -> handleConnectResponse(c, response))
+                            // Close recently created connection in case of any error while it connects to the proxy:
+                            .recoverWith(t -> c.closeAsync().concat(failed(t)));
+                } catch (Throwable t) {


I think we are being overly paranoid here about the calls to c.request() or c.connect() throwing. Any method returning an asynchronous source is not expected to throw. Having said that it is not a big deal so its ok as it is, I will leave it to you to take a call on this.

idelpivnitskiy requested review from NiteshKant and Scottmitch April 4, 2020 00:34

normanmaurer reviewed Apr 5, 2020

View reviewed changes

...-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java Outdated Show resolved Hide resolved

normanmaurer approved these changes Apr 5, 2020

View reviewed changes

NiteshKant suggested changes Apr 6, 2020

View reviewed changes

Address comments

a0cd7b8

idelpivnitskiy commented Apr 7, 2020

View reviewed changes

idelpivnitskiy requested a review from NiteshKant April 7, 2020 15:43

Scottmitch reviewed Apr 7, 2020

View reviewed changes

idelpivnitskiy added 4 commits April 7, 2020 16:37

Merge remote-tracking branch 'upstream/master' into proxy-close-conne…

685900e

…ction

Use whenFinally to prevent handling cancel event after termination

d2bf22f

Do not drain payload body in case of error

083347d

Change whenFinally to afterFinally

cfd0117

idelpivnitskiy mentioned this pull request Apr 8, 2020

Handle cancelation after ConnectionFactory creates a new connection #1010

Open

Remove close-on-cancel

57e470b

NiteshKant reviewed Apr 8, 2020

View reviewed changes

idelpivnitskiy added 2 commits April 8, 2020 16:09

Remove unnecessary try-catch

0144018

Move handleConnectResponse inside ProxyFilter

12cbd4c

idelpivnitskiy merged commit ebb4931 into apple:master Apr 9, 2020

idelpivnitskiy deleted the proxy-close-connection branch April 9, 2020 01:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ProxyConnectConnectionFactoryFilter` leaks connection in case of errors #1002

`ProxyConnectConnectionFactoryFilter` leaks connection in case of errors #1002

idelpivnitskiy commented Apr 4, 2020 •

edited

Loading

idelpivnitskiy Apr 7, 2020

NiteshKant Apr 7, 2020

idelpivnitskiy Apr 7, 2020

Scottmitch Apr 7, 2020

NiteshKant Apr 8, 2020

idelpivnitskiy Apr 8, 2020 •

edited

Loading

NiteshKant Apr 8, 2020

idelpivnitskiy Apr 8, 2020

idelpivnitskiy Apr 7, 2020

Scottmitch Apr 7, 2020

idelpivnitskiy commented Apr 8, 2020

idelpivnitskiy commented Apr 8, 2020

NiteshKant left a comment

NiteshKant Apr 8, 2020

ProxyConnectConnectionFactoryFilter leaks connection in case of errors #1002

ProxyConnectConnectionFactoryFilter leaks connection in case of errors #1002

Conversation

idelpivnitskiy commented Apr 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy commented Apr 8, 2020

idelpivnitskiy commented Apr 8, 2020

NiteshKant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`ProxyConnectConnectionFactoryFilter` leaks connection in case of errors #1002

`ProxyConnectConnectionFactoryFilter` leaks connection in case of errors #1002

idelpivnitskiy commented Apr 4, 2020 •

edited

Loading

idelpivnitskiy Apr 8, 2020 •

edited

Loading