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

Edit report at https://bugs.php.net/bug.php?id=77580&edit=1 ID: 77580 User updated by: oliver dot pfister at contaware dot com Reported by: oliver dot pfister at contaware dot com Summary: DeleteTimerQueueTimer() return code Status: Open Type: Bug Package: Scripting Engine problem Operating System: Windows PHP Version: Irrelevant Block user comment: N Private report: N New Comment: 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? Previous Comments: ------------------------------------------------------------------------ [2019-02-11 17:27:25] ab@php.net 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 retry. With using INVALID_HANDLE_VALUE - i'll do more tests, if you confirm it also works with Wine/ReactOS, it might be the only thing needed. Thanks. ------------------------------------------------------------------------ [2019-02-10 21:52:30] oliver dot pfister at contaware dot com Thanks for the report and detailed analysis. Out of curiosity - I can certainly test Wine, but how do you run ReactOS? Would it work under with Hyper-V or VMware? -> it's possible to run ReactOS in VMWare, VirtualBox or Hyper-V (note that ReactOS supports only the WinXP API, they are working on Vista and higher API support). Disregarding of that, we should not pass when ERROR_IO_PENDING is returned. The timer callback sets flags for Zend VM to recognize the timeout and to properly shutdown. If the timer callback was too slow and didn't set the flags, chances are that ZVM will continue executing some instructions where timeout should happen. -> there is a chance that the callback sets the timed_out global variable after or before it is cleared in zend_set_timeout() or zend_unset_timeout(), I agree that this is not so clean. The handling could be possibly improved by two things: - repeat the call once when GetLastError() == ERROR_IO_PENDING -> repeating the DeleteTimerQueueTimer() call when GetLastError() is ERROR_IO_PENDING is not possible, I tested it, it will rise an exception, and also the Microsoft documentation clearly states that when GetLastError() is ERROR_IO_PENDING the timer is scheduled for deletion as soon as the callback exits, so that we should not delete it again. But we are allowed to create a new timer before the old callback terminates, that works, I tested it. - use INVALID_HANDLE_VALUE for the completion event + care about thread safety -> that's the cleanest solution, but I do not know whether the access to zend_executor_globals *eg is serialized, if yes we have a problem and must take care of that. Note that the INVALID_HANDLE_VALUE solution would also work well in Wine/ReactOS as in that case the implementation is done coherently with Windows. I made a small Win32 application to test the different timer behaviors, if you are interested, tell me. ------------------------------------------------------------------------ [2019-02-08 19:07:05] ab@php.net Thanks for the report and detailed analysis. Out of curiosity - I can certainly test Wine, but how do you run ReactOS? Would it work under with Hyper-V or VMware? Disregarding of that, we should not pass when ERROR_IO_PENDING is returned. The timer callback sets flags for Zend VM to recognize the timeout and to properly shutdown. If the timer callback was too slow and didn't set the flags, chances are that ZVM will continue executing some instructions where timeout should happen. The handling could be possibly improved by two things - use INVALID_HANDLE_VALUE for the completion event + care about thread safety - repeat the call once when GetLastError() != ERROR_IO_PENDING If the timer deletion still fails after that, there's no choice other than dying hard, because in that case we can't ensure the timeout is handled properly. Would it workout on ReactOS? Thanks. ------------------------------------------------------------------------ [2019-02-07 14:21:52] oliver dot pfister at contaware dot com Description: ------------ DeleteTimerQueueTimer(HANDLE TimerQueue, HANDLE Timer, HANDLE CompletionEvent) return code problem in file zend_execute_API.c Because of vague Microsoft API documentation (see point 3. below) there is an important implementation difference between Windows and Wine/ReactOS in case of CompletionEvent set to NULL (PHP uses DeleteTimerQueueTimer with CompletionEvent set to NULL): Windows (tested on all versions of Windows from XP to Win10): calling DeleteTimerQueueTimer returns TRUE if the callback code is not yet executing, then when the thread (of the pool) enters the callback code, calling DeleteTimerQueueTimer returns FALSE with ERROR_IO_PENDING. When the thread (of the pool) has terminated execution of the callback code, calling DeleteTimerQueueTimer returns again TRUE. ReactOS/Wine (tested on latest versions of Wine and ReactOS): calling DeleteTimerQueueTimer always returns FALSE with ERROR_IO_PENDING. Important points of the DeleteTimerQueueTimer Microsoft Windows documentation (https://msdn.microsoft.com/en-us/library/windows/desktop/ms682569(v=vs.85).aspx): 1. If CompletionEvent is NULL, the function marks the timer for deletion and returns immediately. If the timer has already expired, the timer callback function will run to completion. 2. 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. 3. If there are outstanding callback functions and CompletionEvent is NULL, the function will fail and set the error code to ERROR_IO_PENDING. This indicates that there are outstanding callback functions. Those callbacks either will execute or are in the middle of executing. The timer is cleaned up when the callback function is finished executing. As a result in Wine/ReactOS it's not possible to use any PHP, it fails with a fatal error. In Windows there is the possibility that the timer is deleted exactly when a thread (of the pool) is executing the callback, in that case a PHP fatal error would be risen for no good reason. So I propose to correct the PHP code and always check GetLastError() making sure it is not returning ERROR_IO_PENDING before failing. Test script: --------------- In zend_set_timeout_ex() and zend_unset_timeout() please correct the return code check from: if (!DeleteTimerQueueTimer(NULL, tq_timer, NULL)) { to: if (!DeleteTimerQueueTimer(NULL, tq_timer, NULL) && GetLastError() != ERROR_IO_PENDING) { ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=77580&edit=1