Fix concurrency issue in timeout timer value processing #337
Conversation
According to multiprocessing documentation for Value (https://docs.python.org/3/library/multiprocessing.html#multiprocessing.Value) reads and writes are protected with lock when the lock argument is True (the default) or the lock argument is an instance of Lock or RLock. The documentation states that operations like += are not atomic as that involves reading and writing. On the worker side the critical section includes also storing finished task result because the timeout could happen after the task function has finished but before the result has been stored and timer.value has been updated to tell the guard process that the task has been finished. On the guard side the critical section includes all checks done to see if the worker has timed out or died and the actual reincarnation function because the worker could update timer value to -1 (idle) or -2 (recycle) after the guard has seen timer value 0 (timeout) and is going to terminate the worker.
Codecov Report
@@ Coverage Diff @@
## master #337 +/- ##
=========================================
+ Coverage 90.39% 90.4% +<.01%
=========================================
Files 43 43
Lines 2843 2845 +2
=========================================
+ Hits 2570 2572 +2
Misses 273 273
Continue to review full report at Codecov.
|
|
I also spotted issue with the timeout and guard_cycle handling. If guard_cycle is not something that a float can present precisely (like 1.3) timeouts won't work as the current code makes an assumption that timeout is multiple of guard_cycle (this requirement is not documented at all). The following combinations do not work:
@Koed00 Would you accept a PR that would refactor timer handling in such way that state of worker is put into a separate variable and the timer.value is only used for timeouts? |
|
@Koed00 Is there something that needs to be changed or fixed? |
|
Looks good. Thanks for the effort on this one. |
According to multiprocessing documentation for Value
(https://docs.python.org/3/library/multiprocessing.html#multiprocessing.Value)
reads and writes are protected with lock when the lock argument is True
(the default) or the lock argument is an instance of Lock or RLock. The
documentation states that operations like += are not atomic as that
involves reading and writing.
On the worker side the critical section includes also storing finished task
result because the timeout could happen after the task function has
finished but before the result has been stored and timer.value has been
updated to tell the guard process that the task has been finished.
On the guard side the critical section includes all checks done to see
if the worker has timed out or died and the actual reincarnation function
because the worker could update timer value to -1 (idle) or -2 (recycle)
after the guard has seen timer value 0 (timeout) and is going to terminate
the worker.