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

Handle EINTR when doing sysread calls #148

Closed robmueller closed 1 year ago

robmueller commented 1 year ago

Starman usually sets max_spare_servers to max_servers - 1, so in general Net::Server will not try and kill off spare servers.

However if you explicitly set max_spare_servers lower, then Net::Server::PreFork will attempt to do it's usual process management in coordinate_children(), which may end up calling kill_n_children() if there's excess children. This will send a HUP signal to some child processes to ask them to exit.

The code in Net::Server::PreFork tries to only do this for 'waiting' child processes, but notes this is racy. That should be fine, because the child should continue processing any request in progress, and then exit after finishing the request because done() will be true after run_client_connection() completes.

However Starman doesn't currently check the result of sysread() calls for EINTR, so if it receives a HUP while processing a request, the sysread() call returns undef, which ends up propogating up as an error message "Read error: Interrupted system call".

The correct solution here is to detect the EINTR response and redo the sysread() call to complete the entire request successfully, before the high layer exits the child.

rjbs commented 1 year ago

@miyagawa Ooh, thanks for merging this. What's your release schedule like? Obviously we can patch locally, but since there's been so little churn in the last year, it's hard to know if you release regularly after commits, or just when the spirit moves you. 😉

miyagawa commented 1 year ago

This is shipped as 0.4017 to CPAN.

ap commented 1 year ago

Looks like last year started a tradition of a yearly release on Sep 13. 🤭

rjbs commented 1 year ago

Thanks, @miyagawa, I appreciate you!