rq / django-rq

A simple app that provides django integration for RQ (Redis Queue)
MIT License
1.82k stars 286 forks source link

Use django's autoreload feature #262

Open ffigiel opened 6 years ago

ffigiel commented 6 years ago

Autoreloading has been a long-requested RQ feature. Since it seems it's not going to be implemented in a while, I thought that django-rq could use runserver's auto-reloading feature instead.

I looked at how runserver does autoreloading and it's much simpler than I thought. Check it out: https://github.com/django/django/blob/master/django/core/management/commands/runserver.py#L98-L105

Of course this may cause issues with workers being stopped mid-job or some problems across various django versions, but we should at least research this a bit more 😃

I can prepare a PR for this. I assume this would be an opt-in feature, and should display a warning when enabled in production mode

selwin commented 6 years ago

Yeah, a PR would be welcome.

Whether or not workers are stopped mid job depends on how autoreload.main is implemented. If it sends a SIGKILL to the management command, we're golden as RQ will wait until the job is finished before shutting down.

ffigiel commented 6 years ago

I quickly hit a brick wall unfortunately 😞 Looks like we'll need to at least subclass the Worker

worker_1     | Traceback (most recent call last):
worker_1     |   File "/usr/local/lib/python3.6/site-packages/django/utils/autoreload.py", line 228, in wrapper
worker_1     |     fn(*args, **kwargs)
worker_1     |   File "/backend/django-rq/django_rq/management/commands/rqworker.py", line 127, in inner_handle
worker_1     |     w.work(burst=options.get('burst', False))
worker_1     |   File "/usr/local/lib/python3.6/site-packages/rq/worker.py", line 448, in work
worker_1     |     self._install_signal_handlers()
worker_1     |   File "/usr/local/lib/python3.6/site-packages/rq/worker.py", line 363, in _install_signal_handlers
worker_1     |     signal.signal(signal.SIGINT, self.request_stop)
worker_1     |   File "/usr/local/lib/python3.6/signal.py", line 47, in signal
worker_1     |     handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
worker_1     | ValueError: signal only works in main thread

After patching the worker to do nothing in _install_signal_handlers the worker started, but died as soon as I saved a file:

worker_1     | Traceback (most recent call last):
worker_1     |   File "/usr/local/lib/python3.6/site-packages/django/utils/autoreload.py", line 228, in wrapper
worker_1     |     fn(*args, **kwargs)
worker_1     |   File "/backend/django-rq/django_rq/management/commands/rqworker.py", line 132, in inner_handle
worker_1     |     w.work(burst=options.get('burst', False))
worker_1     |   File "/usr/local/lib/python3.6/site-packages/rq/worker.py", line 451, in work
worker_1     |     self.register_birth()
worker_1     |   File "/usr/local/lib/python3.6/site-packages/rq/worker.py", line 260, in register_birth
worker_1     |     raise ValueError(msg.format(self.name))
worker_1     | ValueError: There exists an active worker named '2402' already

Then I added a simple w.register_death() and everything seems to work perfectly 🎉

gif

honi commented 5 years ago

I implemented a wrapper around the rqworker management command (as another management command) that uses django.utils.autoreload. It runs the rqworker using the subprocess module, writing the worker's pid to a temp file, which is then read to kill the old rqworker when reloading. Works only on Linux / macOS.

https://gist.github.com/honi/81ba1bff622649a4a2ef7ccf695b1fa0

Feedback welcomed!

BenEaston commented 5 years ago

Thank you @honi That script works great for me Much appreciated

taewookim commented 4 years ago

@honi

how do you pass the rest of the arguments ?

i.e.. normal way was

python manage.py rqworker

How do I pass the args to your wrapper?

taewookim commented 4 years ago

Nvm. I saw your solution here.