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 reading input in RESTEasy Reactive #15553

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Mar 8, 2021

The idea here is to ensure that Vert.x has not completed reading the input because we can install the proper listeners.
Without this change, the is a chance the input ends before we all the listeners thus resulting in an IllegalStateException being thrown.

Fixes: #15479

@stuartwdouglas
Copy link
Member

I don't really see how this fixes it? If you are on a blocking thread then the pause() should be too late, while on an event loop thread pausing and resuming in the same method should have no effect?

@geoand
Copy link
Contributor Author

geoand commented Mar 9, 2021

I may be wrong, but what I was seeing is that pause ensured that we could add the listeners.
Without it, there was a chance that the request was ended before we add the listeners thus leading to an IllegalStateException being thrown.

Is there a more proper way to get around that problem in Vert.x?

@geoand geoand closed this Mar 9, 2021
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Mar 9, 2021
@geoand geoand reopened this Mar 9, 2021
@geoand geoand removed the triage/invalid This doesn't seem right label Mar 9, 2021
@geoand
Copy link
Contributor Author

geoand commented Mar 9, 2021

After discussing (see https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Resteasy.20Reactive/near/229432044), this fix should be OK for now, but we might need to come up with a more general solution in the future

@gsmet
Copy link
Member

gsmet commented Mar 9, 2021

I will let @stuartwdouglas or @FroMage approve it and merge :).

@geoand
Copy link
Contributor Author

geoand commented Mar 9, 2021

Yup. OP also mentioned that he was going to test it, so let's see how that goes

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.

I saw the discussion and agree this is fine for now.

@FroMage
Copy link
Member

FroMage commented Mar 9, 2021

So waiting for the test before we merge?

@geoand
Copy link
Contributor Author

geoand commented Mar 9, 2021

Let's give OP another half day :)

This was referenced Mar 12, 2021
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.

Resteasy Reactive first POST with InputStream hang
4 participants