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.

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.

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.

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?

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.

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.

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.

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

Many thanks for the collaboration. Closing this for now.

Thanks.