-
Notifications
You must be signed in to change notification settings - Fork 204
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
Fix sse connection infinite retry failure #729
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
connectionResetHandler.call(throwable); | ||
}) | ||
.retryWhen(retryLogic) | ||
.doOnCompleted(this::resetConnected); | ||
.doOnError((Throwable throwable) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having double doOnError
(here and on line 251) seems wrong to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both will still be called, regardless that there is a retryWhen
in between.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me try test this. My understanding is the retryWhen will swallow the error so outer doOnError won't see it if it's being retried
Context
We noticed that when the SSE connection triggered "doOnError", the SSE HTTP client was closed internally (via resetConn) but the following "retryWhen" operation started an infinite retry action on the closed HTTP Client (and every retry failed right away since the client is not going to start the actual connection).
Solution:
Refactor to split reset and close operations on the SSE client so that the "doOnErro" within the retry loop will only reset instead of closing the client. Added an outer "doOnError" to make sure the client gets closed once the retry loop is broken.
Checklist
./gradlew build
compiles code correctly./gradlew test
passes all tests