Bug #77580 [Opn->Csd]: DeleteTimerQueueTimer() return code

Edit report at https://bugs.php.net/bug.php?id=77580&edit=1 ID: 77580 Updated by: ab@php.net Reported by: oliver dot pfister at contaware dot com Summary: DeleteTimerQueueTimer() return code -Status: Open +Status: Closed Type: Bug Package: Scripting Engine problem Operating System: Windows PHP Version: Irrelevant -Assigned To: +Assigned To: ab Block user comment: N Private report: N New Comment: Many thanks for the collaboration. Closing this for now. Thanks. Previous Comments: ------------------------------------------------------------------------ [2019-02-13 22:49:30] oliver dot pfister at contaware dot com I back-ported the INVALID_HANDLE_VALUE patch to my custom PHP 5.6.40 build, it works well under Wine! I read your commit comment, maybe if you can edit it, I would add that the patch fixes a bug: DeleteTimerQueueTimer with the original NULL parameter, returns FALSE (with ERROR_IO_PENDING) if the callback is in execution. For that case the old code fatally failed, but in reality it is not an error condition at all, it is just the way by which DeleteTimerQueueTimer tells you that right now the callback is executing and that the deletion of the timer will happen when the callback terminates. Thanks for all ------------------------------------------------------------------------ [2019-02-13 05:11:25] ab@php.net I pushed a patch with usage of INVALID_HANDLE_VALUE to 7.4+. Could you check the latest master snapshots on Wine/ReactOS please? https://windows.php.net/snapshots/ Regarding the interlocked API - yes, the are atomic but not in the sense of C++. Atomic in this case means that the bits will be written at once, not that the access would be synchronized. As the timer callback runs on a separate thread, at least the main thread can produce a race condition. Such issues have never been reported, but it is theoretically possible. But anyway, it's a separate topic, not to be handled here. Thanks. ------------------------------------------------------------------------ [2019-02-12 22:22:04] oliver dot pfister at contaware dot com Using DeleteTimerQueueTimer in locking mode (with the INVALID_HANDLE_VALUE parameter) guarantees that an old callback which could set timed_out after we reset timed_out in zend_set_timeout() or zend_unset_timeout() cannot exist. For this reason in zend_set_timeout_ex() and zend_unset_timeout() I suggest changing: if (!DeleteTimerQueueTimer(NULL, tq_timer, NULL)) { to: if (!DeleteTimerQueueTimer(NULL, tq_timer, INVALID_HANDLE_VALUE)) { For types which are not bigger than the size of a native integer, if the memory is properly aligned (as it is by default), reads and writes with literals (like setting or resetting a flag) are always atomic. timed_out is of type zend_bool which is an unsigned char, read and writes on that 8-bit variable is always atomic (no alignment restrictions for a single byte). So it's not necessary to use InterlockedExchange. ------------------------------------------------------------------------ [2019-02-12 18:04:08] ab@php.net Ohh, of course it was a mistake, of course repeat when GetLastError() != ERROR_IO_PENDING :) The zend_executor_globals access is not guarded. Still one probably should use InterlockedExchange to set the flag, as the timer runs on a separate thread. Thread safe builds and extensions like pthreads should still operate on an isolated copy of the globals, but safer is safer. The timer callback is supposed to only set the timeout flags, so it should be quite fast. From that POV, probably INVALID_HANDLE_VALUE or NULL shouldn't matter much. And thus, waiting for the callback to complete should not be a big issue. Thanks. ------------------------------------------------------------------------ [2019-02-12 07:45:55] oliver dot pfister at contaware dot com About repeating the call, as from the doc: [doc] If the function fails, the return value is zero. To get extended error information, call GetLastError. If the error code is ERROR_IO_PENDING, it is not necessary to call this function again. For any other error, you should retry the call. [/doc] so with GetLastError() == ERROR_IO_PENDING one should NOT retry to call DeleteTimerQueueTimer again as the timer has been scheduled for deletion by the DeleteTimerQueueTimer call (if DeleteTimerQueueTimer is called again an unrecoverable Windows exception is risen). With all other error codes php must fail with a fatal error, like already done now. Back to the idea of using INVALID_HANDLE_VALUE: Passing that parameter will force DeleteTimerQueueTimer to wait the termination of the callback if at the time of the DeleteTimerQueueTimer call the callback is executing. If we call DeleteTimerQueueTimer before the timer expires then the timer is deleted without any wait. If we call DeleteTimerQueueTimer after the timer callback has been executed then the timer is deleted without any wait. So the possible death lock remains only when we call DeleteTimerQueueTimer while the callback is executing (the callback is executed by a system thread), this is why I asked you whether the access to zend_executor_globals *eg is controlled by a critical section/mutex, is it the case? ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=77580 -- Edit this bug report at https://bugs.php.net/bug.php?id=77580&edit=1