miyagawa / Starman

Starman is a high-performance preforking Perl PSGI web server
http://search.cpan.org/dist/Starman
Other
287 stars 84 forks source link

Starman does not support setting port to `0` #119

Open frioux opened 9 years ago

frioux commented 9 years ago

I would like to have Starman get a port from the kernel that is ensured to be open. The correct way to do this is to set the port to 0 and the kernel will assign a free port to the socket. If you tell plackup to use port 0 (or listen 0:0) it will just use one of the predefined ports, instead of letting the kernel choose.

I suspect that the fix for this involves checking definedness instead of truthiness on the value you get back, but I'm not sure.

miyagawa commented 9 years ago
➜  Plack git:(master) plackup --listen 0:0 -s Starman eg/dot-psgi/Hello.psgi
2015/11/05-09:03:48 Starman::Server (type Net::Server::PreFork) starting! pid(98705)
Missing port in hashref passed in port argument.

Net::Server does not seem to allow this usage, so I'd close it as wontfix.

ap commented 9 years ago
$ plackup --listen 0:00 -s Starman eg/dot-psgi/Hello.psgi
2015/11/05-19:09:14 Starman::Server (type Net::Server::PreFork) starting! pid(72530)
Resolved [*]:00 to [0.0.0.0]:00, IPv4
Binding to TCP port 00 on host 0.0.0.0 with IPv4
Setting gid to "XX XX XX XX XX XX XX XX XX XX XX XX XX"
Starman: Accepting connections at http://*:00/
rjbs commented 9 years ago

Net::Server does allow port 0.

~$ perl -MNet::Server -e 'package X { use base "Net::Server"; sub process_request {  ... } } X->run(port => 0)'
2015/11/05-13:14:18 X (type Net::Server) starting! pid(32898)
Resolved [*]:0 to [0.0.0.0]:0, IPv4
Binding to TCP port 0 on host 0.0.0.0 with IPv4
  Bound to auto-assigned port 57361
Group Not Defined.  Defaulting to EGID '20 20 12 61 399 100'
User Not Defined.  Defaulting to EUID '503'
miyagawa commented 9 years ago

plackup --listen 0:00 -s Starman eg/dot-psgi/Hello.psgi

Ha. I was trying 0:0 and it got an error so I was thinking there's a code in PreFork that doesn't allow port 0. The "Accepting connections at" bit looks weird though, it should at least tell me which port it actually is bound, otherwise the server ready callback is useless and we can't tell which port it's actually listening on.

rjbs commented 9 years ago

It's weird, right? 00 works because it's being counted true somewhere, but Net::Server doesn't report back the used port with 00, but it does display it with port 0. I'm guessing there are two bugs here — one that prevents 0 from being accepted because it's false, and one that prevents displaying the kernel-chosen port when 00 is used (and so port is true).

ap commented 9 years ago

Correct. I do think this is a Net::Server bug and not a Starman bug (and so @miyagawa was right to close it), though.

There’s a line in Net::Server::Proto that prevents passing port 0 through the form of the port parameter that Starman uses. I don’t know why that check is there. As far as I can tell the test suite does not test it, and there doesn’t seem to be anywhere else in Net::Server that doesn’t like port 0. Hell, if you use a different form of listen spec and omit the port, that very same function does $info->{'port'} ||= 0 right after that block. The function appears confused about its pre- and post-conditions.

Based on a guess at what the function is trying to validate, it should instead look like this:

But this might break existing code that works (albeit with warnings). The other option (which I’d prefer) is to punt:

I’m not sure why I’m writing this here instead of the Net::Server RT queue.