Code review comment for lp://qastaging/~jamesj/openobject-server/complex-cron-fix

Hi,

Thanks for proposing changes as code!

Unfortunately, the patch doesn't seem right to me.

Leaving aside the fact things as still named 'threads' in various places (including in the command line options and in the docstrings), there are truly implementation details that are thread-specific that you have not removed/changed:
  - the possibility to cancel wakeups (and I guess that once a task is pushed to the multiprocessing.Queue, it doesn't make sense to alter its cancel attribute) and the related variables and locks
  - the old runner_body that you have replaced is still there

But those are details; the biggest problem is that the main change (i.e. using a process-friendly Queue) has no purpose here:

You use a master cron process, instead of a master cron thread. But you still spawn worker *threads*. The difference is those will run inside the master *cron* process instead of the *main*
application process. Thus there is no need for a process-friendly Queue: the worker threads can still communicate with the main cron thread (the main thread of the cron process).

This leaves us with the question of starting the main cron thread in its own process or not (i.e. around the line 80s in the patch). I am not against that choice but actually don't see any benefits doing so.

The real problem with the current implementation (and still present in your patch) is that the cron system is not aware of changes that happen in database because of other processes. Say a worker process creates a new task, in the current implementation, the wakeup structure is updateed and shared with the master thread. If that communication is broken, the master thread will never spawn a new worker. A possibility to solve the problem is to poll the database to see if anything has change. This seems a very good solution when the number of database to monitor is limited.