Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add --reload option for code reloading #682

Merged
merged 2 commits into from Jan 29, 2014

Conversation

Projects
None yet
3 participants
Collaborator

tilgovi commented Jan 16, 2014

Fix #526

Collaborator

tilgovi commented Jan 16, 2014

Welcome questions / comments / changes.

Collaborator

tilgovi commented Jan 16, 2014

Wasn't sure whether to do the callback or pass the log in order to log the message. Also willing to consider a server hook for code change as the callback instead, or as another feature.

Collaborator

tilgovi commented Jan 16, 2014

Also, I don't know if the NOTICE change is necessary. I was the principal author of greins, but I didn't write the reloader first (even though I've modified it here, and it's simple, and similar to common code in many places and examples for python module watching). Anyway, it wasn't (c) me, so I included it.

Collaborator

tilgovi commented Jan 16, 2014

Still needed:

  • test interaction with paster, which has its own --reload
  • see if i can easily add the config file to the watched files
Collaborator

tilgovi commented Jan 16, 2014

Stand by... I will move the reloader support to the base application, and hook in at load_config_from_file

Owner

benoitc commented Jan 28, 2014

@tilgovi patch looks good for me. We should indeed see how to handle it with paster. Maybe there is a way to disable paster?

About the config file, what do you mean ? The gunicorn config file? Shouldn't we let this one out of the reloading? Imo HUP is enough for such purpose.

@tilgovi tilgovi added a commit that referenced this pull request Jan 29, 2014

@tilgovi tilgovi Merge pull request #682 from tilgovi/feature/526
Add --reload option for code reloading
d4f2481

@tilgovi tilgovi merged commit d4f2481 into benoitc:master Jan 29, 2014

1 check was pending

default The Travis CI build is in progress
Details

@tilgovi tilgovi deleted the tilgovi:feature/526 branch Jan 29, 2014

@georgexsh georgexsh commented on the diff Feb 8, 2014

gunicorn/reloader.py
+
+
+class Reloader(threading.Thread):
+ def __init__(self, extra_files=None, interval=1, callback=None):
+ super(Reloader, self).__init__()
+ self.setDaemon(True)
+ self._extra_files = set(extra_files or ())
+ self._extra_files_lock = threading.RLock()
+ self._interval = interval
+ self._callback = callback
+
+ def add_extra_file(self, filename):
+ with self._extra_files_lock:
+ self._extra_files.add(filename)
+
+ def get_files(self):
@georgexsh

georgexsh Feb 8, 2014

Contributor

This default reloader implementation seems not appeal to me

  • could not deal add new file
  • scan every source file in sys.modules is obvious unnecessary, and there is no guarantee all app modules would be loaded when fire Reloader.get_files, so the reloader scanned extra files but missed some
  • each worker will do file scan and reload(kill), it is a overhead
Collaborator

tilgovi commented Feb 11, 2014

@georgexsh I welcome improvements.

You can add new files. Maybe you mean that the reloader instance is never stored so add_extra_file cannot be called.

I don't see a better way than scanning everything in sys.modules. I can't see why it would be important to limit it to a specific package. I don't know what you mean by "reloader scanned extra files but missed some". Can you say more?

I'm not at all bothered by all workers checking. It's overhead, but this is for development, not production.

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