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

Use unused port for registry #21

Closed
wants to merge 11 commits into from

Conversation

ciderale
Copy link

This PR introduces a script to select an unused port for the registry. This should essentially address #10.

The solution has a dependency on lsof, but since nc is used already, I think that should be okay.
Moreover, the registry is terminated via a trap ... EXIT, so it is shutdown even in case of unexpected errors.

@nmattia
Copy link
Collaborator

nmattia commented Sep 22, 2020

Hi, thanks for the PR!

Scanning ports to find one open usually has two big drawbacks: it is not atomic (the port may get assigned by the time the registry starts) and it's somewhat slow. I would rather use port 0 and have the registry report the actual port the OS gave it. If you need this ASAP @ciderale I'm happy to merge; otherwise we could look into using port 0?

@ciderale
Copy link
Author

Thanks for the quick reply.

You are right, the operation is not atomic. Concerning speed, it is not a port scanning if I understand that correctly and it is rather fast. lsof -i -P -n 0.02s user 0.08s system 90% cpu 0.106 total.

I shortly thought about port 0, but I didn't want to modify the registry. I also don't know, if that works everywhere, so I went for listing the ports instead. But yes, that is probably the cleaner solution.

I don't need it asap. I was investigating with nix&npm and your package works nicely. I was looking into separating the creation of node_modules and the actual package build (as mentioned elsewhere afaik), and this was the first little thing to do.

Alain Lehmann added 4 commits September 23, 2020 00:33
- for some reasons, using port 0 does select a random port,
but npm fails when retrieving data from it (does it crash?)
@ciderale
Copy link
Author

ciderale commented Sep 22, 2020

@nmattia I made an update that now binds to a random port.

Unfortunately, using port 0 did not work. While it listened on a random port, npm was not able to get the data from the registry without errors. I'm not sure what the reason is. Therefore, I used the Warp.openFreePort to achieve the goal. This seems to work well. (tested with the "deckdeckgo-starter" derivation on a mac)

Another challenge was to get the port that has been allocated. I first thought to retrieve it somehow from stdout. But that seems not that trivial since the application continues to run. As we anyway have to wait for the registry to start, I don't think retrieving the information with lsof is much different from nc.

Let me know what you think about the new solution.

aszlig added a commit to aszlig/napalm that referenced this pull request Oct 17, 2020
This is an alternative solution to pull request nix-community#21, which implements
picking a random TCP port for the registry. While I haven't found the
exact reason why it has to be randomized, I can only guess that it's to
prevent port conflicts in unsandboxed builds.

The implementation also has few ugly workarounds (eg. using lsof to get
the port number), but the main issue I see is that even if the port is
random, any user/process on the system can still connect to that port.

So instead of picking a random TCP port, let's simply not use IP sockets
at all and use ip2unix to force NPM into connecting to the Unix socket.

We're now using a dummy IP address (127.0.0.100) for the registry URL in
order to be able to match the registry URL in the ip2unix rule as a
distinct host and at the same time prevent silent failure in case we
forgot to transform/wrap sockets at some point.

Signed-off-by: aszlig <aszlig@nix.build>
Fixes: nix-community#4, nix-community#10
@nmattia
Copy link
Collaborator

nmattia commented Oct 21, 2020

Closing in favor of #24, thanks for the inspiration!

@nmattia nmattia closed this Oct 21, 2020
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.

2 participants