Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix in-process race condition #36

Merged
merged 8 commits into from
Dec 18, 2019

Conversation

davidmarkclements
Copy link
Contributor

This is related to #23 but it's doesn't solve the multiprocess race condition.

This might have only started occurring in later node versions, but when calling get-port a lot sometimes the same port will be returned and then port already bound error occurs when trying to bind to it.

You can extend this to solve #23 by replacing Set with an ondisk store (maybe leveldb?).

The reason for using an old/young sets approach is to avoid setting one interval per port (which would be overkill imo) while also ensuring that a port isn't forgotten for at least the sweep time, but up to double the sweep time.

@davidmarkclements davidmarkclements force-pushed the master branch 3 times, most recently from 629710a to 6198da5 Compare November 24, 2019 19:33
@sindresorhus
Copy link
Owner

Thanks for working on this 🙌

Can you add a test?

Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
@davidmarkclements
Copy link
Contributor Author

davidmarkclements commented Dec 10, 2019

thanks @sindresorhus for the feedback, should be all addressed now 👍

oops - except for test, incoming

@davidmarkclements
Copy link
Contributor Author

@sindresorhus ok test added, also realised that we only need one setInterval and it's lazily initialised

@sindresorhus
Copy link
Owner

The code looks good. Can you mention the behavior in the readme? That ports are locked up, how long, why, and how it helps prevent race-conditions.

@sindresorhus sindresorhus changed the title resolve in-process race condition Fix in-process race condition Dec 16, 2019
@davidmarkclements
Copy link
Contributor Author

@sindresorhus description of behaviour has been added to the readme 👍

@sindresorhus sindresorhus merged commit 73e21d8 into sindresorhus:master Dec 18, 2019
@sindresorhus
Copy link
Owner

Excellent. Thanks for working on this :)

@davidmarkclements
Copy link
Contributor Author

thanks @sindresorhus !

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

Successfully merging this pull request may close these issues.

2 participants