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.

Sign up for GitHub

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

Jump to bottom

5049 fix pipe race condition #5050

Open
wants to merge 2 commits into
base: 4.x
Choose a base branch
from
Open

5049 fix pipe race condition #5050

wants to merge 2 commits into from

Conversation

NilsRenaud
Copy link
Contributor

Fixes the issue #5049

@NilsRenaud
eclipse-vertx#5049 fix Pipe's race condition
3bb8d6c 
Fix issue: eclipse-vertx#5049

Signed-off-by: Nils Renaud <renaud.nils@gmail.com>
@NilsRenaud
Fix a broken test in PipeTest
f0cb637 
PipeTest#testFailStreamPrematurely was failing because the error handler is now activated before receiving the first element of the read stream.

But, I think that's an error in the FakeStream implementation which calls the failure handler immediately, bypassing the content delivery.
This means that the previous FakeThread implementation permitted:
- multiple failure handler calls on the same Stream
- A failure event THEN a new element read
This, IMO does not respect the ReadStream behaviour.

This test was only OK because the failure handler was set after the call to resume in the Pipe implementation. But it looks like a tweak to accommodate with the buggy FakeStream behavior.

Signed-off-by: Nils Renaud <renaud.nils@gmail.com>
@NilsRenaud
Copy link
Contributor Author

NilsRenaud commented Jan 3, 2024 •
edited

There is still a broken test RecordParserTest.testWrapReadStream, because a test explicitly calls FakeStream#write() after FakeStream#fail() I thought it wasn't possible...
Is this an error or are stream failures expected to happen multiple times on the same streams, even between 2 elements ?

@vietj vietj added this to the 4.5.2 milestone Jan 8, 2024
@NilsRenaud
Copy link
Contributor Author

@vietj what do you think of my previous comment, is RecordParserTest.testWrapReadStreambuggy ?

@vietj vietj modified the milestones: 4.5.2, 4.5.3 Jan 30, 2024
@vietj vietj modified the milestones: 4.5.3, 4.5.4 Feb 6, 2024
@vietj vietj modified the milestones: 4.5.4, 4.5.5 Feb 22, 2024
@vietj vietj modified the milestones: 4.5.5, 4.5.6 Mar 14, 2024
@vietj vietj modified the milestones: 4.5.6, 4.5.7, 4.5.8 Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.5.8
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@NilsRenaud @vietj