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

Rely on the kernel for port allocation #498

Merged
merged 4 commits into from Apr 5, 2022
Merged

Rely on the kernel for port allocation #498

merged 4 commits into from Apr 5, 2022

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Apr 5, 2022

The testing infrastructure requires that a substrate binary is present for individual testing.
However, having a different polkadot binary running resulted in failing tests.

Root Cause

The function next_open_port() in subxt crate produced 3 port numbers by inspecting
a range of ports.
To validate that a given port is opened the TcpListener::bind(("0.0.0.0", 0)) bind call is executed.
However, the address "0.0.0.0" is not representative of the localhost, but of a non-routable unknown address.

This observation is validated using the polkadot binary which starts by default on 127.0.0.1:9944. Any TcpListener::bind(("0.0.0.0", 0)) will result in success even with the binary started.

Port Allocation

While at it, remove the scanning logic and rely entirely on the Kernel to provide 3 available ports.
The port allocation is performed by the inet_csk_get_port() function.
When providing a zero port, asking for allocation, the code path goes to inet_csk_find_open_port().
A port scanning is performed that uses a spinlock for concurrent access with the addition of checking for bind conflicts.

The kernel should provide 3 different ports if they are available.

Testing Done

Started 100 threads, each requesting for 100 new ports. Multiple runs of the binary presented no duplicates.
However, when requesting more ports than available (ie from 1000 threads), the ports are reutilized and values eventually wrap from net.inet.ip.portrange.last to net.inet.ip.portrange.first.

Closes #497.

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Excellent! Simpler and more reliable; the ideal combo :)

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice 👍

@lexnv lexnv merged commit cd4c64c into master Apr 5, 2022
@lexnv lexnv deleted the 497_port_alloc branch April 5, 2022 17:57
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.

Test failing when having a polkadot binary started
3 participants