Enforce a finite bound on the time gap between signal receipt and signal handler execution.

[gnossen]

While drafting #19465, I found that the following simple snippet of code did not work:

stub = foo_pb2_grpc.FooStub(channel)
result_generator = stub.StreamingRPC(foo_pb2.FooReques())

def cancel_request(unused_signum, unused_frame):
    result_generator.cancel()

signal.signal(signal.SIGINT, cancel_request)
for result in result_generator:
    do_stuff(result)

Signal handlers were only given a chance to run upon receipt of an entry in the RPC stream. Since there is no time bound on how long that might take, there can be an arbitrarily long time gap between receipt of the signal and the execution of the application's signal handlers.

It turns out that this issue was not limited to streaming RPCs. Unary RPCs exhibited the same property.

Signal handlers are only run on the main thread. The cpython implementation takes great care to ensure that the main thread does not block for an arbitrarily long period between signal checks.

Our indefinite blocking was due to wait() invocations on condition variables without a timeout.

Related:

• Signal Handlers not Run While Client Iterates over Server-Streaming or Bidi RPC #19464
• Add Python Cancellation Example #19465

[gnossen]

Manually verified that this allows for the desired simplifications in #19465, but I'm still working on unit tests. Just wanted to get eyes on this. @lidizheng

[lidizheng]

I encountered the same problem with wait in #19299.

I personally prefer setting infinite timeout than periodically check, because IMO the later solution might have greater overhead while spinning.

[gnossen]

@lidizheng If we want to be responsive to handlers, some sort of spinning is going to be necessary. It's just a question of which layer it's going to be happening in. This is the code path that is exercised when you supply a timeout to wait(). However, reading this code, I can't help but feel that the C path is going to be faster than running this code in Python. It looks like the "infinite" timeout is going to be set to a spin with a period of 20 milliseconds. The infinite timeout looks like a good solution as long as we pair it with a test that verifies that cpython exhibits the behavior we're expecting.

Edit: I stand corrected. That second link I posted is for those platforms that do not have their own sem_timedwait. It looks like this behavior is dependent on the implementation of each individual platform. Perhaps we should actually quantify any changes in performance on the data path?

[lidizheng]

LGTM to the design! Good work!

Please take a look at failed test cases.

[lidizheng]

Optional: if we move this function to Cython layer, we can gain free performance boost to reduce the overhead of introducing this spin wait mechanism.

[lidizheng]

Also, should we add a skip condition if the _common.wait is not ran in main thread? Then it won't block signal handling?

[gnossen]

@lidizheng I considered it, but I'm wondering if checking the TID on every call to wait would be more costly than just adding the timeout unconditionally. I'll do some more research on this.

[gnossen]

@lidizheng Thinking about it a little bit more, I don't think it makes sense to only add this behavior to the main thread. Suppose we're on some other thread and the application blocks indefinitely in a C-level function while holding the GIL (e.g. waiter.acquire()). Since it has the GIL, the main thread still will not be able to execute the signal handler.

[gnossen]

So I went ahead and disabled the gevent tests for this PR, as I'm bumping up against #18980. On a separate branch, I've determined the root cause and have a fix that makes this test pass under gevent (though perhaps not the ideal fix). My plan is to merge this to master and then remove the test from the blocklist in the follow-up PR that addresses #18980.

[lidizheng]

That makes sense. Thank you for thinking through this optimization.

…

On Wed, Jul 3, 2019 at 13:59 Richard Belleville ***@***.***> wrote: @lidizheng <https://github.com/lidizheng> Thinking about it a little bit more, I don't think it makes sense to only add this behavior to the main thread. Suppose we're on some other thread and the application blocks indefinitely in a C-level function while holding the GIL (e.g. waiter.acquire()). Since it has the GIL, the main thread *still* will not be able to execute the signal handler. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19481?email_source=notifications&email_token=ABYNM4BIBHVII2WVTLOUJIDP5UALBA5CNFSM4H3V3472YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZFQZHY#issuecomment-508234911>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYNM4DVPSISZ5ID4L7EO6DP5UALBANCNFSM4H3V347Q> .

[gnossen]

@lidizheng PTALA.

[gnossen]

#19554