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

Fix sse connection infinite retry failure #729

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Conversation

Andyz26
Copy link
Collaborator

@Andyz26 Andyz26 commented Dec 5, 2024

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
  • Added new tests where applicable
  • ./gradlew test passes all tests
  • Extended README or added javadocs where applicable

@Andyz26 Andyz26 had a problem deploying to Integrate Pull Request December 5, 2024 23:45 — with GitHub Actions Failure
@Andyz26 Andyz26 changed the title Fix sse connection infinite retry from onError Fix sse connection infinite retry failure Dec 5, 2024
Copy link

github-actions bot commented Dec 5, 2024

Test Results

615 tests  +120   605 ✅ +116   7m 53s ⏱️ +51s
142 suites + 20    10 💤 +  5 
142 files   + 20     0 ❌  -   1 

Results for commit 877a8a9. ± Comparison against base commit 880d2fc.

Copy link
Contributor

@sundargates sundargates left a 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) -> {
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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

@Andyz26 Andyz26 merged commit 0419367 into master Dec 6, 2024
4 of 5 checks passed
@Andyz26 Andyz26 deleted the andyz/sseRetryLoop branch December 6, 2024 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants