ryanhiebert / hirefire

A Python lib to integrate with the HireFire service -- The Heroku Proccess Manager.
https://hirefire.readthedocs.io/
Other
34 stars 21 forks source link

load_procs being called two times. #16

Closed ihucos closed 8 years ago

ihucos commented 8 years ago

In our setup that class attribute definition gets execute two times: https://github.com/jezdez/hirefire/blob/e29ef21ffdece81967117b12a73692b37887e28f/hirefire/contrib/django/middleware.py#L33

I can reproduce our exception using reload on the middleware module and don't know any other way how that line can get evaluated multiple times. So the question is maybe if Django middlewares are supposed to survive a reload or not.

Our fix was to just don't complain when procs gets loaded multiple times. https://github.com/resmio/hirefire/commit/3bb23433a34ef73b3604ee5536c1305ade8b842a

This is our traceback:

[2016-06-28 18:27:01 +0000] [169] [ERROR] Error handling request 
Traceback (most recent call last): 
  File "/app/.heroku/python/lib/python2.7/site-packages/gunicorn/workers/sync.py", line 130, in handle 
    self.handle_request(listener, req, client, addr) 
  File "app/.heroku/python/lib/python2.7/site-packages/gunicorn/workers/sync.py", line 171, in handle_request 
    respiter = self.wsgi(environ, resp.start_response) 
  File "/app/.heroku/python/lib/python2.7/site-packages/newrelic-2.50.0.39/newrelic/api/web_transaction.py", line 1329, in _nr_wsgi_application_wrapper_ 
    result = wrapped(*args, **kwargs) 
  File "/app/.heroku/python/lib/python2.7/site-packages/newrelic-2.50.0.39/newrelic/api/web_transaction.py", line 1329, in _nr_wsgi_application_wrapper_ 
    result = wrapped(*args, **kwargs) 
  File "/app/.heroku/python/lib/python2.7/site-packages/django/core/handlers/wsgi.py", line 170, in __call__ 
    self.load_middleware() 
  File "/app/.heroku/python/lib/python2.7/site-packages/newrelic-2.50.0.39/newrelic/common/object_wrapper.py", line 302, in _wrapper 
    result = wrapped(*args, **kwargs) 
  File "/app/.heroku/python/lib/python2.7/site-packages/django/core/handlers/base.py", line 52, in load_middleware 
    mw_instance = mw_class() 
  File "/app/.heroku/python/lib/python2.7/site-packages/opbeat/contrib/django/middleware/__init__.py", line 107, in __init__ 
    self.instrument_middlewares() 
  File "/app/.heroku/python/lib/python2.7/site-packages/opbeat/contrib/django/middleware/__init__.py", line 115, in instrument_middlewares 
    module = import_module(module_path) g
  File "/app/.heroku/python/lib/python2.7/importlib/__init__.py", line 37, in import_module 
    __import__(name) 
  File "/app/.heroku/python/lib/python2.7/site-packages/hirefire/contrib/django/middleware.py", line 25, in <module> 
    class HireFireMiddleware(object): 
  File "/app/.heroku/python/lib/python2.7/site-packages/hirefire/contrib/django/middleware.py", line 33, in HireFireMiddleware 
    loaded_procs = load_procs(*PROCS) 
  File "/app/.heroku/python/lib/python2.7/site-packages/hirefire/procs/__init__.py", line 57, in load_procs 
    (proc, loaded_procs[proc.name])) 
ValueError: Given proc <Proc worker: 'resmioproject.celeryapp.WorkerProc'> overlaps with another already loaded proc (<Proc worker: 'resmioproject.celeryapp.WorkerProc'>) 
ryanhiebert commented 8 years ago

I know that I personally haven't had any use-case where reloading the module would make sense, and it would be unsurprising to me that Django would have problems with it in general. Would you be willing to expand your use-case for reloading the modules?

ihucos commented 8 years ago

Would you be willing to expand your use-case for reloading the modules?

Unfortunately I could not find what does the reload, the exception could also be caused by something completely else. My guess is, that it is gunicorn trying to respawn when the celery server could no be reached, and that possibly related to the use of "--preload". But I could not reproduce that. So really no idea.

mpetyx commented 8 years ago

i think i have the exact same issue, causing responses on production to take more than 4 seconds and i had to migrate back on version 0.3...

ryanhiebert commented 8 years ago

@mpetyx : If moving back to 0.3 fixed your issue, than that's more likely because of the default for Celery Procs to inspect all the types of tasks on the workers. For me, and what seemed like the typical use-case, not inspecting those caused innaccurate data, which was a bigger problem than the additional response time.

If the trade-off is right for you, then you can add inspect_statuses = [] to your Proc definition to signal that tasks that are already owned by workers (running, reserved, or scheduled) should not be counted.

For example:

MyProc(CeleryProc):
    name = 'my_proc'
    queues = ['my_queue']
    inspect_statuses = []

This is documented at https://hirefire.readthedocs.io/en/latest/procs/#celery

I hope that helps. If you confirm that it is indeed an issue with procs being registered multiple times, then please let us know your use-case for that ability, as it doesn't make much sense to us yet.

mpetyx commented 8 years ago

i would have to check this on production level, so it may take some time. thank you very much for your quick reply!

ryanhiebert commented 8 years ago

I'm closing this, as it appears that it's likely invalid. If you disagree, feel free to make a case to re-open it. For now, I don't know that it's reasonable to assume that Django Middleware (or other things, like models) can reasonably be reloaded.