[lldb] Something wrong with new MainLoopWindows for pipe support

[slydiman]

The PR #145621 added the new class PipeEvent and the test NoSpuriousPipeReads.
This test is flaky:

https://lab.llvm.org/buildbot/#/builders/211/builds/231
https://lab.llvm.org/buildbot/#/builders/211/builds/274
https://lab.llvm.org/buildbot/#/builders/211/builds/284

https://lab.llvm.org/buildbot/#/builders/197/builds/6659
https://lab.llvm.org/buildbot/#/builders/197/builds/6690
https://lab.llvm.org/buildbot/#/builders/197/builds/6701
https://lab.llvm.org/buildbot/#/builders/197/builds/6735
https://lab.llvm.org/buildbot/#/builders/197/builds/6737

It causes problems with production buildbots and must be fixed ASAP.
I'm not sure disabling this test is a good solution.

[llvmbot]

@llvm/issue-subscribers-lldb

Author: Dmitry Vasilyev (slydiman)

Th PR #145621 added the new class PipeEvent and the test NoSpuriousPipeReads. This test is flaky:

https://lab.llvm.org/buildbot/#/builders/211/builds/231
https://lab.llvm.org/buildbot/#/builders/211/builds/274
https://lab.llvm.org/buildbot/#/builders/211/builds/284

https://lab.llvm.org/buildbot/#/builders/197/builds/6659
https://lab.llvm.org/buildbot/#/builders/197/builds/6690
https://lab.llvm.org/buildbot/#/builders/197/builds/6701
https://lab.llvm.org/buildbot/#/builders/197/builds/6735
https://lab.llvm.org/buildbot/#/builders/197/builds/6737

It cause problems with production buildbots and must be fixed ASAP.
I'm not sure disabling this test is good solution.

[slydiman]

@labath

[labath]

I think it's fine to disable the test to avoid buildbot noise, but I agree this should be fixed. The spurious reads can make it hard to use this correctly.

Looking at the implementation I think this is a race between the polling thread (which starts a new iteration as soon as Disarm is called -- and that's before the callback is run) and the callback itself. I think that kicking of the polling thread just before we select (around WillPoll) should fix that. I guess you'd need an extra flag to tell if the pipe has been signalled already. It might also be possible to use sticky (non-auto-resetting) events for that, though I'm not sure if that would be cleaner.

[slydiman]

BTW, why is WSAWaitForMultipleEvents still used instead of WaitForMultipleObjects for non-socket handles?

[ashgti]

WSAWaitForMultipleEvents is calling WaitForMultipleObjectsEx per the Remarks section of https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsawaitformultipleevents#remarks and the lphEvents param supports taking different HANDLE types. WSAEVENT is defined as #define WSAEVENT HANDLE in the headers so they can be used in the same places in a few different APIs.

I'll take a look at this and see if I can come up with a fix.