Skip to content

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