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 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.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Thanks for working on this 🙌

Can you add a test?

index.js Outdated Show resolved Hide resolved
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 !

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

2 participants