[Lldb-commits] [PATCH] D133181: [test] Ensure MainLoop has time to start listening for signals.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

> AFAICT kill is entirely asynchronous

This is not exactly true. POSIX describes this situation quite well (emphasis mine):

> If the value of pid causes sig to be generated for the sending process, and if sig is not blocked for the calling thread and if no other thread has sig unblocked or is waiting in a sigwait() function for sig, **either sig or at least one pending unblocked signal shall be delivered to the sending thread before kill() returns**.

Our problem is that this sentence does not apply here, not for one, but two reasons:

- "sig is not blocked for the calling thread" -- the calling thread in fact has the signal blocked. That is expected, as it will be unblocked (and delivered) in the `ppoll` call inside `MainLoop::Run`. That is a pretty good way to catch signals race-free, but it also relies on the another part of the sentence above.
- "no other thread has sig unblocked" -- it only works if there are no other threads willing to accept that signal, and I believe that is what is failing us here. This test does in fact create an extra thread in its SetUp function on line 35. By the time we leave the SetUp function, that thread has finished with its useful work (producing the future object), but I suspect what is happening is that, occasionally, the OS-level thread fails to exit on time and eats our signal.

In this case, I believe that the simplest way to fix this is to get rid of that thread. I believe it was necessary at some point in the past (when we were doing the Listen+Accept calls as a single action), but now it is not necessary as the self-connection can be completed without having two threads actively connecting to each other -- it's enough that one socket declares its intent to accept (listen to) a connection. That will make the test simpler. and I believe it will also fix the flakyness you observed.



================
Comment at: lldb/unittests/Host/MainLoopTest.cpp:35
     Socket *accept_socket;
     std::future<Status> accept_error = std::async(std::launch::async, [&] {
       return listen_socket_up->Accept(accept_socket);
----------------
Thread created here.


================
Comment at: lldb/unittests/Host/MainLoopTest.cpp:45
     ASSERT_TRUE(error.Success());
     ASSERT_TRUE(accept_error.get().Success());
 
----------------
Delete the async call above, and put something like `ASSERT_TRUE(listen_socket_up->Accept(accept_socket).Success())` here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133181/new/

https://reviews.llvm.org/D133181