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 race condition on reactive client with streaming and SSE responses #16438

Merged
merged 2 commits into from Apr 13, 2021

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Apr 12, 2021

Fixes: #16227

@geoand
Copy link
Contributor Author

geoand commented Apr 12, 2021

I used JUnit's @RepeatedTest(1000) (and even then did multiple runs) locally to ensure that these tests are not flaky any more with this fix

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

So hold on, the fix is which part? Is it a double call to complete() or a double call to close(true)? Both?

IIRC closeHandler was called when the server dropped the connection without ending/responding it. I'm not sure we have tests in place for this case (not sure how we could).

@geoand
Copy link
Contributor Author

geoand commented Apr 12, 2021

The PR fixes 2 different cases - one for streaming chunks and one for SSE (which correspond to the 2 re-enabled tests).

In both cases it could happen that the closeHandler of the connection was being called before the handler for the items

@FroMage
Copy link
Member

FroMage commented Apr 12, 2021

Oh, close before handler for items. Damn. Huh. Oh well, I guess we can fix this and wait until any bug related to connection dying appear later. But, then please add a comment in both cases to explain why there's no closeHandler pointing to this issue because we'll never remember to not add it ;)

@geoand
Copy link
Contributor Author

geoand commented Apr 12, 2021

Will do

@geoand
Copy link
Contributor Author

geoand commented Apr 12, 2021

PR updated with the request comment

@geoand geoand requested a review from FroMage April 12, 2021 14:43
Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 12, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 12, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building a236231

Status Name Step Test failures Logs Raw logs
Devtools Tests - JDK ${{ matrix.java.name }} ⚠️ Check →
Devtools Tests - JDK 11 Windows ⚠️ Check →
Gradle Tests - JDK 11 ${{ matrix.os.family }} ⚠️ Check →
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK ${{ matrix.java.name }} ⚠️ Check →
JVM Tests - JDK 11 Windows ⚠️ Check →
Maven Tests - JDK ${{ matrix.java.name }} ⚠️ Check →
Maven Tests - JDK 11 Windows ⚠️ Check →
MicroProfile TCKs Tests ⚠️ Check →
Native Tests - ${{ matrix.category }} ⚠️ Check →
Native Tests - Windows - ${{ matrix.category }} ⚠️ Check →

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 12, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building d634926

Status Name Step Test failures Logs Raw logs
Devtools Tests - JDK ${{ matrix.java.name }} ⚠️ Check →
Devtools Tests - JDK 11 Windows ⚠️ Check →
Gradle Tests - JDK 11 ${{ matrix.os.family }} ⚠️ Check →
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK ${{ matrix.java.name }} ⚠️ Check →
JVM Tests - JDK 11 Windows ⚠️ Check →
Maven Tests - JDK ${{ matrix.java.name }} ⚠️ Check →
Maven Tests - JDK 11 Windows ⚠️ Check →
MicroProfile TCKs Tests ⚠️ Check →
Native Tests - ${{ matrix.category }} ⚠️ Check →
Native Tests - Windows - ${{ matrix.category }} ⚠️ Check →

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

But worried about closing.

@geoand
Copy link
Contributor Author

geoand commented Apr 12, 2021

Yeah, we'll have to see...

@gsmet
Copy link
Member

gsmet commented Apr 12, 2021

Rebased to get the latest CI fixes.

@gastaldi gastaldi merged commit eb169e8 into quarkusio:main Apr 13, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 13, 2021

Milestone is already set for some of the items:

We haven't automatically updated the milestones for these items.

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone Apr 13, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 13, 2021
@gastaldi gastaldi deleted the #16227 branch April 13, 2021 03:29
@gsmet gsmet removed this from the 2.0 - main milestone Apr 13, 2021
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.

Investigate io.quarkus.resteasy.reactive.server.test.stream.StreamTestCase#testClientStreaming
5 participants