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 - [PATCH] hid_read_timeout() has hidden 100 ms sleep on ARM64 causing infinite loop in callers
Summary: [PATCH] hid_read_timeout() has hidden 100 ms sleep on ARM64 causing infinite ...
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: joystick (show other bugs)
Version: 2.0.14
Hardware: x86 Windows 10
: P2 normal
Assignee: Sam Lantinga
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-02 00:12 UTC by Cameron Gutman
Modified: 2021-01-02 18:14 UTC (History)
0 users

See Also:


Attachments
Patch (2.49 KB, patch)
2021-01-02 00:14 UTC, Cameron Gutman
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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