cenkalti / kuyruk

⚙️ Simple task queue for Python
https://kuyruk.readthedocs.org/
MIT License
231 stars 17 forks source link

Windows support #54

Closed frol closed 6 years ago

frol commented 6 years ago

Trying to start Kuyruk on Windows end up with an error due to the missing signals support on Windows:

Traceback (most recent call last):
  File "c:\project\tasks\app\run.py", line 26, in run
    app.run()
  File "c:\Anaconda\Win64\envs\env\lib\site-packages\kuyruk\worker.py",
ine 97, in run
    signal.signal(signal.SIGHUP, self._handle_sighup)
AttributeError: module 'signal' has no attribute 'SIGHUP'

I am going to try comment out all the signals usage, but even if that works, I believe, it will miss quite a few features... Any ideas on how to work this around for Windows?

P.S. Is Windows support something that you are interested in?

cenkalti commented 6 years ago

I don't know much about the signalling system on Windows, so I don't plan supporting Windows, sorry :(

frol commented 6 years ago

Well, if you don't mind, I will share my findings as to the running Kuyruk on Windows.

I had commented out the ALARM signal usage in the task.py:time_limit (thus, the time limit feature is in the broken state), and also SIGHUP, SIGUSR1, and SIGUSR2 in the worker.py:Worker.run (I am not exactly sure what it breaks, but I am sure it breaks something :)). These are all the changes I needed to make in Kuyruk to make it working on Windows!

NOTE: There is also a compatibility issue in py-amqp (https://github.com/celery/py-amqp/issues/135), but a simple patch, which skips setting TCP_MAXSEG on Windows, solved the issue. Just a simple amqp upgrade (2.2.2) got everything sorted.

Given the surface of the work, I can prepare a PR to enable Windows support. Will you be so kind to review/accept it once it is ready?

cenkalti commented 6 years ago

Of course!

frol commented 6 years ago

Here are my findings on the topic:

  1. (just a restatement) there is no signals/process timeout concept (signal.alarm() just isn't supported at all with no alternative provided)
  2. There are plenty of thread-based "solutions" in the wild, but all of them are broken due to the simple fact that Threads cannot be stopped in Python and, in fact, it is a bad idea to do that even if possible (e.g. in C) - https://eli.thegreenplace.net/2011/08/22/how-not-to-set-a-timeout-on-a-computation-in-python
  3. signals.alarm() doesn't play well with threads, and the timeout value is rounded to seconds precision (e.g. timeout=0.6 => timeout=0)
  4. Using multiprocessing is the only clean solution in terms of time limiting, but it has quite a significant overhead, and given that Windows doesn't support Unix forking, it is a hassle to work with multiprocessing on Windows.
  5. Given everything stated above, I would avoid tasks.py:time_limit implementation altogether (even for Unix!), and leave it up to kuyruk user to implement the timeout the way they like it.

As to the singals handling in worker.py:Worker:

  1. There is no way to implement SIGHUP / SIGUSR1 / SIGUSR2 on Windows transparently.
  2. SIGUSR1 (prints traceback) and SIGUSR2 (drops a current task) are used for debug purposes, so we can just disable these features for Windows (nobody likes Windows either way :))
  3. The use of SIGHUP to interrupt a running task in case of lost RabbitMQ connect is quite agressive.

Let's consider the solutions:

  1. I propose to raise a NotImplementedError in the tasks.py:time_limit for Windows if the seconds != 0, thus stating clearly that it is not going to work on Windows.
  2. Simply avoid SIGUSR1 and SIGUSR2 handlers on Windows.
  3. Implement a cooperative API for task termination instead of intrusive SIGHUP, but as a temporary solution, we can simply ignore the case of lost RabbitMQ connection on Windows...