Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure `TcpConnector#connect` does not emit error after success #967

Merged
merged 4 commits into from Mar 17, 2020

Conversation

@NiteshKant
Copy link
Member

NiteshKant commented Mar 13, 2020

Motivation

TcpConnector#connect eagerly terminates the Single when the channel is registered. It also terminates the Single with failure if connect fails.
This violates the contract of the Subscriber that there should only be a single termination signal.

Modification

As we have to complete the returned Single with the channel as soon as the channel is registered to ensure we do not miss channel events and we also have to propagate connect failures, we can not abide by the Single<Channel> contract. Instead of returning Single<Channel> now returning Single<Connection> and do the translation of Channel -> Connection using a supplied Function.

Result

TcpConnector#connect abides by the Subscriber contract.

__Motivation__

`TcpConnector#connect` eagerly terminates the `Single` when the channel is registered. It also terminates the `Single` with failure if connect fails.
This violates the contract of the `Subscriber` that there should only be a single termination signal.

__Modification__

As we have to complete the returned `Single` with the channel as soon as the channel is registered to ensure we do not miss channel events and we also have to propagate connect failures, we can not abide by the `Single<Channel>` contract. Instead of returning `Single<Channel>` now returning `Single<Connection>` and do the translation of `Channel` -> `Connection` using a supplied `Function`.

__Result__

`TcpConnector#connect` abides by the `Subscriber` contract.
@NiteshKant NiteshKant requested review from idelpivnitskiy and Scottmitch Mar 13, 2020
Future<?> connectFuture = connect0(localAddress, resolvedRemoteAddress, config, autoRead,
executionContext, connectHandler);
connectHandler.connectFuture(connectFuture);
connectFuture.addListener(f -> {

This comment has been minimized.

Copy link
@idelpivnitskiy

idelpivnitskiy Mar 16, 2020

Member

Consider verifying if connectFuture is already done before applying the listener, as we do in other places:

final Future<List<InetAddress>> addressFuture = resolver.resolveAll(inetHost);
cancellableForQuery = () -> addressFuture.cancel(true);
if (addressFuture.isDone()) {
handleResolveDone0(addressFuture);
} else {
addressFuture.addListener((FutureListener<List<InetAddress>>) this::handleResolveDone0);
}

This comment has been minimized.

Copy link
@NiteshKant

NiteshKant Mar 17, 2020

Author Member

We do that when we are already on the eventloop and no ordering guarantees are required. In this case, adding a listener makes sure that the Single is completed on the eventloop.

(This code was just refactored, so I would continue with the approach what was present before but if you feel we need to change this in general in all places, we can discuss/do that separately)

@@ -244,15 +244,16 @@ public void onSubscribe(final Cancellable cancellable) {
}

@Override
public void onSuccess(final C connection) {
requireNonNull(connection);
public void onSuccess(@Nullable final C connection) {
if (terminatedUpdater.compareAndSet(ConnectHandler.this, 0, 1)) {
target.onSuccess(connection);

This comment has been minimized.

Copy link
@idelpivnitskiy

idelpivnitskiy Mar 17, 2020

Member

Do we assume that downstream target Subscriber will handle null instead of a connection? I think it's easy to assume from the caller side that it's expected to receive non-null connection. Was suggesting to invoke target.onError instead of throwing from onSuccess.

This comment has been minimized.

Copy link
@NiteshKant

NiteshKant Mar 17, 2020

Author Member

Do we assume that downstream target Subscriber will handle null instead of a connection?

Yes, in general in operators we pass-through what we get unless we use it.

Was suggesting to invoke target.onError instead of throwing from onSuccess.

I understood the suggestion but I am following the general pattern I mention above.

Copy link
Member

normanmaurer left a comment

LGTM... just two nits.

@NiteshKant NiteshKant merged commit c4d421f into apple:master Mar 17, 2020
3 checks passed
3 checks passed
pull request validation (jdk11) Build finished.
Details
pull request validation (jdk8) Build finished.
Details
pull request validation (quality) Build finished.
Details
@NiteshKant NiteshKant deleted the NiteshKant:connector-fix branch Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.