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

Send root-cause errors from connection to onClose of RSocket #797

Merged
merged 3 commits into from
Apr 26, 2020

Conversation

OlegDokuka
Copy link
Member

@OlegDokuka OlegDokuka commented Apr 25, 2020

This PR is a followup to previous RSocketFactory#errorConsumer deprecation and future removal.
That said, we have to have access to the reason of RSocket termination and right now it would be the best to enhance Mono<Void> onClose and propagate the root-cause error (e.g Stream ID 0 errors which are terminal) over that API

Signed-off-by: Oleh Dokuka shadowgun@i.ua

Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good.

One thought is that the onClose of the responder will not be used as far as I can see (RSocketResponder is created without being accessed). I suppose nothing wrong with making request and responder consistent still.

rsocket-core/src/main/java/io/rsocket/Closeable.java Outdated Show resolved Hide resolved
@@ -619,6 +623,10 @@ private void tryTerminateOnKeepAlive(KeepAlive keepAlive) {
String.format("No keep-alive acks for %d ms", keepAlive.getTimeout().toMillis())));
}

private void tryTerminateOnConnectionClose(Throwable e) {
tryTerminate(() -> e);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tryTerminateOnConnectionError perhaps?

}
private void tryTerminateOnConnectionClose(Throwable e) {
tryTerminate(() -> e);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tryTerminateOnConnectionError?

OlegDokuka and others added 2 commits April 26, 2020 12:50
Co-Authored-By: Rossen Stoyanchev <rstoyanchev@users.noreply.github.com>
Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
@rstoyanchev rstoyanchev changed the title changes Closable to send the root-cause error via Mono Send root-cause errors from connection to onClose of RSocket Apr 26, 2020
@OlegDokuka OlegDokuka merged commit 414193f into develop Apr 26, 2020
@OlegDokuka OlegDokuka deleted the future/terminal-via-on-close branch April 26, 2020 12:50
@OlegDokuka OlegDokuka added this to the 1.0 RC7 milestone Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants