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

pserve --reload with --daemon #912

Closed
mmerickel opened this issue Mar 14, 2013 · 10 comments
Closed

pserve --reload with --daemon #912

mmerickel opened this issue Mar 14, 2013 · 10 comments

Comments

@mmerickel
Copy link
Member

I'm not sure if this is supposed to work, but tizoc in #pyramid claimed it may be a problem. If this is supposed to work we should verify that it does. If it is not supposed to work (which I think would be fine) we should add an error message when trying to execute pserve with both options enabled.

@mcdonc
Copy link
Member

mcdonc commented Mar 14, 2013

It is not supposed to work AFAIK.

@mmerickel
Copy link
Member Author

https://github.com/mmerickel/pyramid/compare/fix.pserve-monitor-daemon

Contains a fix adding support for the full blown combination of --reload --monitor-restart --daemon.

@digitalresistor
Copy link
Member

Would this also solve the problem if the reload fails due to a Python exception? Right now I have pserve for development sitting in a loop with --reload so that if it fails, it sleep 10 and retries. This way I don't need to go restart it when I save a file and miss a comma or something along those lines.

@mmerickel
Copy link
Member Author

This does allow for that. Although it attempts to restart with no delay, so we'd probably want to add a throttle to the --monitor-restart option or combine the logic such that we "don't restart until a file changes". A more ideal implementation would be something like https://pypi.python.org/pypi/ReloadWSGI which does not crash when your application fails to start, but continues to serve the old version. I had issues using ReloadWSGI on OS X last time I tried, and I think that it has no windows support at all.

@digitalresistor
Copy link
Member

At least in my case, I have no need for it to continue serving the old version, but having it wait till a file has changed after failure would probably be best.

@joulez
Copy link

joulez commented Aug 11, 2013

Slightly related (getting daemon functionality to be slightly more managable)
The pserve daemon feature needs some clean up, particularly when checking for a PID (which should be done early on). I'm not sure of the benefits of read_pidfile and live_pidfile being seperate functions? It seems to add some complexity (only care about a valid PID).
But this will break some test scripts that check for 1) The pid file 2) That the file contains a value 3) That the value is a valid PID.
Is the pserve daemon functionality and the subcommands widely used? {start|stop|restart|status}

@piotr-dobrogost
Copy link

From what I see --reload does not work with --daemon in Pyramid 1.4.5
What's preventing @mmerickel's fix from getting merged?

@mcdonc
Copy link
Member

mcdonc commented Apr 13, 2015

My $.02: I'd rather get rid of --daemon and --stop-daemon than trying to make pserve a full-blown process manager or add complexity to make daemon-stopping work with --monitor-restart. Very good process managers already exist and pserve doesn't really need to do their jobs.

That said, if @mmerickel wants to merge the patch, I'd be fine with it.

@mmerickel
Copy link
Member Author

I'm a big fan of removing --daemon but it may be controversial.

Side note: my fix is something I would not consider production ready due to the infinite loop caused by saving a file with broken code while --monitor-restart is active.

@digitalresistor
Copy link
Member

I'd be all for removing --daemon. I never understood why we try to be everything with pserve. Then again, I run on top of uWSGI.

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

No branches or pull requests

5 participants