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

breaking test for fd pool #38

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gmaclennan
Copy link

The file descriptor pool does not work as expected when increasing the number of RAF instances. I believe this is caused by a race condition where the RAF is not removed from active on the same tick, so suspending a random RAF in active can result in the same RAF being suspended more than once.

@gmaclennan
Copy link
Author

Added an alternative Pool implementation that passes the tests. It's probably not as performant as the original implementation, but it does pass the test.

@gmaclennan
Copy link
Author

A bit more about what is happening in the original code:

When _onactive() is called and the pool is full, suspend() is called on a random RAF instance. However it does not suspend in the same tick - it goes into a "suspending" state before entering the "suspended" state. _oninactive() is not called until the instance enters the "suspended" state.

This means that if _onactive is called again (e.g. if another RAF instance is opened) while a RAF instance is in the "suspending" state, then it will still be in the active array, so there is a chance it will be randomly selected for calling suspend() a second time. This results in more file descriptors being open than maxSize.

The original tests did not catch this because it only creates 3 instances with a pool size of 2, which means that only one instance will ever be suspended. To expose the issue you need at least maxSize+2 instances, but it could still sometimes pass. The test just adds more instances to expose the issue.

The fix I found was to remove from the active array immediately on suspending, however this means that _oninactive can be called when it is no longer in the active array, so the splice technique will not work - it would remove the wrong instance from active. (_oninactive is still important for instances that are closed manually rather than suspended because the pool is full)

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.

None yet

1 participant