We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 5433

Summary: [PATCH] hid_read_timeout() has hidden 100 ms sleep on ARM64 causing infinite loop in callers
Product: SDL Reporter: Cameron Gutman <cameron.gutman>
Component: joystickAssignee: Sam Lantinga <slouken>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P2    
Version: 2.0.14   
Hardware: x86   
OS: Windows 10   
Attachments: Patch

Description Cameron Gutman 2021-01-02 00:12:57 UTC
hid_read_timeout() calls GetOverlappedResult(bWait=TRUE) after WaitForSingleObject(). This is problematic because the event is auto-reset, therefore the call to WaitForSingleObject() resets the event which GetOverlappedResult() will try to wait on.

Even though the overlapped operation is guaranteed to be completed at the point we call GetOverlappedResult(), it will still wait on the event handle for a short time to trigger the reset for auto-reset events. This amounts to roughly a 100 ms sleep each time GetOverlappedResult() is called for a completed I/O with a non-signalled event.

In the context of HIDAPI, this extra sleep means that callers that loop on hid_read_timeout() with timeout=0 will loop forever, since the 100 ms sleep each iteration ensures ReadFile() will always have new data. This means SDL will never return from a call to SDL_PumpEvents().

Logging of time spent in a handful of GetOverlappedResult() calls:
GetOverlappedResult took 93 ms
GetOverlappedResult took 110 ms
GetOverlappedResult took 94 ms
GetOverlappedResult took 109 ms
GetOverlappedResult took 94 ms
GetOverlappedResult took 93 ms
GetOverlappedResult took 110 ms
GetOverlappedResult took 94 ms
GetOverlappedResult took 109 ms

Stack of the GetOverlappedResult calls:
02 KERNELBASE!GetOverlappedResultEx
03 SDL2!PLATFORM_hid_read_timeout
04 SDL2!hid_read_timeout
05 SDL2!HIDAPI_DriverPS5_UpdateDevice
06 SDL2!HIDAPI_UpdateDevices
07 SDL2!SDL_JoystickUpdate_REAL
08 SDL2!SDL_PumpEvents_REAL
09 SDL2!SDL_WaitEventTimeout_REAL

Original user report: https://github.com/moonlight-stream/moonlight-qt/issues/350#issuecomment-753368058

I was able to reproduce this locally on a Surface Pro X with an ARM64 build of SDL 2.0.14 and a PS4 controller plugged in. I believe it is related to the compatibility behavior change for apps declaring Windows 7 compatibility in their manifest that is mentioned on https://docs.microsoft.com/en-us/windows/win32/win7appqual/compatibility---application-manifest in the GetOverlappedResult section. The fix for this race condition is what causes that extra wait to happen (to reset the auto-reset events). I suspect with some tweaks to the application manifest, I could get it to happen on x86/x64 too.
Comment 1 Cameron Gutman 2021-01-02 00:14:12 UTC
Created attachment 4625 [details]
Patch
Comment 2 Sam Lantinga 2021-01-02 18:14:45 UTC
Patch added, thanks!
https://hg.libsdl.org/SDL/rev/04b86016f011