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 ExtEventLoop to keep track of stream resources (refcount) #123

Merged
merged 2 commits into from
Dec 1, 2017

Conversation

clue
Copy link
Member

@clue clue commented Dec 1, 2017

It has come to our attention that certain loop implementations do not keep track of assigned stream resources (refcount). This means that stream resources may be garbage collected despite being added to the loop when leaving the scope where they have been defined. Arguably, this is a design decision that makes sense for a number of PHP extension modules.

This issue is implicitly avoiding by using react/stream, which stores the stream resource in a member variable and thus prevents garbage collection. This means that this is not an issue for most people that rely on this component. However, this issue can be exhibited quite easily by using the EventLoop directly.

Given that our loop interface tries to abstract these underlying differences, we may have to keep track of stream resources within our loop abstractions.

@clue clue changed the title [WIP] Loop implementations have to keep track of stream resources (refcount) Loop implementations have to keep track of stream resources (refcount) Dec 1, 2017
@clue clue added this to the v0.5.0 milestone Dec 1, 2017
@clue
Copy link
Member Author

clue commented Dec 1, 2017

Removed the WIP marker as this PR now contains a minimal work around and we can no longer reproduce this issue with this fix applied (see also Travis output) :shipit:

@clue clue removed the help wanted label Dec 1, 2017
@clue clue changed the title Loop implementations have to keep track of stream resources (refcount) Fix ExtEventLoop to keep track of stream resources (refcount) Dec 1, 2017
Copy link
Member

@jsor jsor left a comment

Choose a reason for hiding this comment

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

👍

@jsor jsor merged commit 7bc0172 into reactphp:master Dec 1, 2017
@clue clue deleted the refcount branch December 1, 2017 12:50
}

/**
* Telper for above test. This happens in another helper method to verify
Copy link

Choose a reason for hiding this comment

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

Helper typo

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.

4 participants