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

HUP doesn't restore the arguments properly because of the use of $0 #34

Closed
miyagawa opened this Issue Sep 6, 2011 · 9 comments

Comments

Projects
None yet
2 participants
@miyagawa
Owner

miyagawa commented Sep 6, 2011

With Plack 0.9971's $0 change, the HUP signal handling is broken, and the way Net::Server retrieves the original command line args.

See also: https://github.com/miyagawa/Plack/issues/243

@frankyd

This comment has been minimized.

Show comment
Hide comment

frankyd commented Sep 6, 2011

I have an example of how to reproduce this:

https://gist.github.com/1197586

https://gist.github.com/1197876

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Oct 11, 2011

Owner

Note that this may or may not work with procfs because of the way Net::Server works. A quick test shows that there's no issue with Mac OS X.

Owner

miyagawa commented Oct 11, 2011

Note that this may or may not work with procfs because of the way Net::Server works. A quick test shows that there's no issue with Mac OS X.

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Dec 1, 2011

Owner

I keep failing to reproduce this bug on Mac, and also hear issues from linux users (I think). Can someone specify 100% reproducible simple step to replicate this bug? I do have a linux environment to test and fix if there's any.

Owner

miyagawa commented Dec 1, 2011

I keep failing to reproduce this bug on Mac, and also hear issues from linux users (I think). Can someone specify 100% reproducible simple step to replicate this bug? I do have a linux environment to test and fix if there's any.

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Dec 1, 2011

Owner

OK i could reproduce now - is it correct that it only happens on a) linux, b) when you use PSGI file other than app.psgi and c) when you use --preload-app.

Owner

miyagawa commented Dec 1, 2011

OK i could reproduce now - is it correct that it only happens on a) linux, b) when you use PSGI file other than app.psgi and c) when you use --preload-app.

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Dec 1, 2011

Owner

Sort of related, Net::Server does this:

delete $ENV{$_} for $self->hup_delete_env_keys;
sub hup_delete_env_keys { return qw(PATH) }

which means if you run starman using just 'perl', relying on your $PATH configured correctly, then sending HUP could potentially run the process again from a wrong path. This is issue number one.

Owner

miyagawa commented Dec 1, 2011

Sort of related, Net::Server does this:

delete $ENV{$_} for $self->hup_delete_env_keys;
sub hup_delete_env_keys { return qw(PATH) }

which means if you run starman using just 'perl', relying on your $PATH configured correctly, then sending HUP could potentially run the process again from a wrong path. This is issue number one.

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Dec 1, 2011

Owner

I think restarting the server with exec $0, @ARGV is not exactly what we want - we should override Net::Server's behavior of handling HUP signals. Also the code to get the original command line in Net::Server is just insane:
https://metacpan.org/source/RHANDOM/Net-Server-0.99/lib/Net/Server.pm#L162

http://unicorn.bogomips.org/SIGNALS.html handles HUP to restart config files, then restart all workers. It will pick up code changes unless the preload_app is used, which we could replicate in Starman as well.

If you use --preload-app then the code changes won't be picked up, which could be confusing to some people. We can document that in --preload-app that you can run starman behind Server::Starter to make HUP to fully restart the server while preloading the app entirely.

Owner

miyagawa commented Dec 1, 2011

I think restarting the server with exec $0, @ARGV is not exactly what we want - we should override Net::Server's behavior of handling HUP signals. Also the code to get the original command line in Net::Server is just insane:
https://metacpan.org/source/RHANDOM/Net-Server-0.99/lib/Net/Server.pm#L162

http://unicorn.bogomips.org/SIGNALS.html handles HUP to restart config files, then restart all workers. It will pick up code changes unless the preload_app is used, which we could replicate in Starman as well.

If you use --preload-app then the code changes won't be picked up, which could be confusing to some people. We can document that in --preload-app that you can run starman behind Server::Starter to make HUP to fully restart the server while preloading the app entirely.

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Dec 2, 2011

Owner

0.29_01 is shipped to CPAN. Test it.

Owner

miyagawa commented Dec 2, 2011

0.29_01 is shipped to CPAN. Test it.

@frankyd

This comment has been minimized.

Show comment
Hide comment
@frankyd

frankyd Feb 9, 2012

I have tested version 0.29_90 and it works exactly as you have described above and in the documentation. A HUP signal now successfully restarts the workers. Thanks for sorting this out.

frankyd commented Feb 9, 2012

I have tested version 0.29_90 and it works exactly as you have described above and in the documentation. A HUP signal now successfully restarts the workers. Thanks for sorting this out.

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Oct 31, 2012

Owner

Closing.

Owner

miyagawa commented Oct 31, 2012

Closing.

@miyagawa miyagawa closed this Oct 31, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment