Skip to content

Fix separately removing readable and writable side of stream when closing #139

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

Merged
merged 2 commits into from
Dec 22, 2017

Conversation

clue
Copy link
Member

@clue clue commented Dec 19, 2017

Filing this as WIP in order to create a reproducible test first and will fix this in a subsequent commit.

Resolves / closes #136

@clue clue added the bug label Dec 19, 2017
@clue clue added this to the v0.5.0 milestone Dec 19, 2017
@clue clue force-pushed the closing branch 6 times, most recently from d59a203 to 1416302 Compare December 19, 2017 16:42
@clue clue force-pushed the closing branch 2 times, most recently from c87b9fa to af89737 Compare December 19, 2017 17:41
@clue clue changed the title [WIP] Fix removing both readable and writable side of stream when closing Fix separately removing readable and writable side of stream when closing Dec 19, 2017
@clue
Copy link
Member Author

clue commented Dec 19, 2017

After analyzing the stack traces in #136, we now have a very good understanding of what is happening:

  • A stream resource is both waiting to write outgoing data and accepting incoming data
  • The event loop notifies the stream is readable
  • The stream component detects a stream is closed by the remote end (strlen(fread()) === 0)
  • The stream component first removes the read listener from the event loop (and after this would also remove the write listener)
  • The event loop tries to set the read-write stream listener to writable only
  • ext-event complains that the stream is not a valid resource

I've added a number of tests as part of this PR to trigger this specific situation. However, while several tests now cover this specific situation, I've been unable to create a reliable, reproducible test where ext-event actually complains.

This means that we have a good understanding of what is failing and we can work around this issue. The updated code avoids this situation by using two separate stream watchers for the readable and writable side that can be removed independently instead of using a single watcher that needs to be updated.

The updated code has 100% code coverage and while I'm currently unable to provide a test case for the initial issue, I believe to have fixed a relevant bug anyway.

Accordingly, I've removed the WIP marker for now :shipit:

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@jsor jsor merged commit 57cb83e into reactphp:master Dec 22, 2017
@clue clue deleted the closing branch December 22, 2017 14:56
@clue
Copy link
Member Author

clue commented Dec 24, 2017

For the reference: This is still a relevant bug fix, but we've finally fixed the root cause via reactphp/socket#141 👍

The issue boils down to a bug where react/socket issued fclose() on the stream resource before passing it to removeReadStream().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event::set(): supplied resource is not a valid
3 participants