Add wait_for_termination method to grpc.Server #19299

lidizheng · 2019-06-08T00:40:25Z

An attempt to add a "run_forever" like method to grpc.Server.

gRFC: L54: Python Server Wait API proposal#149
Tests: Added.
Fixes: Python equivalent of C++ server's Wait() method #17452

gnossen

Looks nice! This will be a great improvement! Three high-level thoughts:

This needs to be our new canonical form. No more time.sleep(SECONDS_IN_A_DAY). We need to do a sweep of our example code and documentation, both internally and externally.
This is definitely going to require a cherrypick.
A change in the abstract interfaces got my spidey senses tingling. We've seen trouble in the past with opencensus and grpcio-testing when making this sort of change. I did a cursory look through grpcio-testing and this looks like it should be okay. There might be another use that I've missed though.

gnossen · 2019-06-10T17:47:34Z

src/python/grpcio/grpc/__init__.py

+        2) The `__del__` of the server object is invoked.
+
+        Args:
+          grace: A duration of time in seconds or None.


Can we define more clearly what grace does?

The grace variable is removed. Now, the wait_for_termination function accepts no argument.

My original design is to replace our internal API which have both delay and grace. However, the semantic will become a lot messy. And it is hard to explain that:

The grace variable set here is not necessary the source of truth, because other thread can call server.Stop as well;

The delay variable will only work in main thread, what should it do in other thread;

The semantic of delay is hard to define that which signal should it react to.

src/python/grpcio/grpc/_server.py

gnossen

Looks good! Just a couple of nits.

gnossen · 2019-06-13T16:31:42Z

src/python/grpcio_tests/tests/unit/_server_wait_for_termination_test.py

+        time.sleep(_WAIT_FOR_BLOCKING.total_seconds())
+
+        # Invoke manually here, in Python 2 it will be invoked by GC sometime.
+        server.__del__()


I think del server is more conventional.

del server is not going to trigger __del__ stably in Python 2, and won't trigger __del__ in Python 3 unless it is the very last ref.

gnossen · 2019-06-13T16:33:50Z

src/python/grpcio/grpc/__init__.py

+        This is an EXPERIMENTAL API.
+
+        The wait will not consume computational resources during blocking, and it
+        will block indefinitely. There are two ways to unblock:


Nit: It's a bit contradictory to say it will block indefinitely and then to define to conditions under which it will stop blocking. How about "...and it will block until one of the two following conditions are met:"

Thank you for the advice. This is much better!

mehrdada · 2019-06-13T17:59:32Z

Why not return the threading.Event and let the user wait on it however they want?

server.termination_event().wait()

Sounds simpler to implement and with less book-keeping (no arrays and locks etc. needed). Simply create a single Event object for each server in __init__ and save it in server state and call set on it when the server shuts down and call it a day. You don't need to raise exception etc. either. Semantics are fully clear because threading.Event has proper docs.

P.S. there's one valid reason not to expose the event object directly, which is the user can screwup and set or clear the event. I am not too worried as this is python and private APIs are just a suggestion anyway, but if you are worried about that, here's my suggestion:

Define wait_for_termination(timeout=None) exactly matching the semantics of threading.Event (even directly link to the doc on threading.Event.wait).
Implement it the same way as above, waiting on a single Event instance you create at the beginning, and proxy the arguments directly to that instance's wait method.

mehrdada · 2019-06-13T18:40:12Z

src/python/grpcio/grpc/__init__.py

+        will block indefinitely. There are two ways to unblock:
+
+        1) Calling `stop` on the server in another thread;
+        2) The `__del__` of the server object is invoked.


__del__ should never appear in the docs. It's an implementation detail and not part of the guaranteed public interface of the object. We can choose to kill __del__ should we want to.

Agreed. Removed.

mehrdada · 2019-06-13T18:40:43Z

src/python/grpcio/grpc/_server.py

+
+        with self._state.lock:
+            if self._state.stage is _ServerStage.STOPPED:
+                raise ValueError('Failed to wait for a stopped server.')


Why raise an exception at all. If we are already stopped it should just return.

Make sense. And it still align with the semantic of blocking until the server is stopped.

lidizheng · 2019-06-13T22:49:31Z

@mehrdada The first change you gave might be too much. As you said, the user might abuse the new API. I think the second suggestion is valid that adding a timeout variable to this function to simulate a Event.wait.

New tests added. Docstring updated. Changes reflected to gRFC.

mehrdada

Thanks--I may have not been clear about my implementation suggestion, so I wrote them down in-line with code

mehrdada · 2019-06-13T23:01:07Z

src/python/grpcio/grpc/_server.py

@@ -764,7 +764,7 @@ def __init__(self, completion_queue, server, generic_handlers,
        self.interceptor_pipeline = interceptor_pipeline
        self.thread_pool = thread_pool
        self.stage = _ServerStage.STOPPED
-        self.shutdown_events = None
+        self.shutdown_events = []


Change to:

self.termination_event = threading.Event() self.shutdown_events = [self.termination_event]

What's the use case you see in terms of giving applications access of the actual Event object?

This is not giving applications access to Event; it's just simplifying your implementation (no locks, just a single event to keep track of all waiters). All of this belongs to the _state object, which is private, no?

That make sense. Changed.

mehrdada · 2019-06-13T23:01:53Z

src/python/grpcio/grpc/_server.py

@@ -959,6 +958,17 @@ def add_secure_port(self, address, server_credentials):
    def start(self):
        _start(self._state)

+    def wait_for_termination(self, timeout=None):
+        termination_event = threading.Event()


Change to:

def wait_for_termination(self, timeout=None): return self._state.termination_event.wait(timeout=timeout)

The return is also important, because it helps the user identify if timeout expired or not

Good catch. This is my oversight.

mehrdada

Thanks looks good now. Have you manually tested an example under the new API to ensure it works well with Ctrl+C?

lidizheng · 2019-06-13T23:34:51Z

@mehrdada Interestingly, the Event.wait is only interrupt-able if the timeout is specified in both Python 2/3. I have manually confirmed this bahavior.

https://bugs.python.org/issue35935

mehrdada · 2019-06-13T23:36:59Z

If this interferes with signals in production and examples alike, what's the point of this feature?
Sounds like in practice you're kind of screwed if you use it. We'd just be increasing the API surface for unclear benefits.

lidizheng · 2019-06-13T23:39:13Z

@mehrdada Alternatively, we can set the default timeout to the maximum timestamp.
https://stackoverflow.com/questions/45704243/what-is-the-value-of-c-pytime-t-in-python

mehrdada · 2019-06-13T23:41:59Z

@lidizheng does that help with Ctrl+C? I assume not. The solution is to do it in a loop with small timeouts, but that sounds bad too. I think you should just axe this feature. Is there a concrete ask here?

lidizheng · 2019-06-14T00:01:01Z

@gnossen @mehrdada It does help with CTRL+C. With the timeout set, most of the problem go away.

Does CTRL+C work for Event.wait:

Platform	Python Version	With `timeout`	Without `timeout`
Windows	2.7	Yes	No
Windows	3.7.1	No	No
macOS	2.7	Yes	No
macOS	3.7	Yes	No
Linux	2.7	Yes	No
Linux	3.6	Yes	Yes
Linux	3.7	Yes	Yes

I would say setting the timeout to a big enough number is a valid workaround for the CPython bug.

lidizheng added kind/enhancement lang/Python release notes: yes Indicates if PR needs to be in release notes labels Jun 8, 2019

gnossen reviewed Jun 10, 2019

View reviewed changes

lidizheng marked this pull request as ready for review June 12, 2019 22:24

gnossen approved these changes Jun 13, 2019

View reviewed changes

mehrdada suggested changes Jun 13, 2019

View reviewed changes

mehrdada approved these changes Jun 13, 2019

View reviewed changes

lidizheng force-pushed the wait branch from 91e399d to 44c0418 Compare June 14, 2019 00:20

lidizheng force-pushed the wait branch from 44c0418 to 8a5f65e Compare June 26, 2019 16:53

lidizheng mentioned this pull request Jun 26, 2019

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

Merged

lidizheng added 8 commits August 2, 2019 14:56

Add wait_for_termination method to grpc.Server

9f6243e

Add a unit test for wait_for_termination

7257508

Make sanity test happy

c4b7831

"EXPERIMENTAL API" added in docstring

8f6ee97

Add timeout argument to wait_for_termination

59555d5

Simplify implementation

518b55a

Add a workaround for CPython issue 35935

42e8bd2

Revert changes to helloworld example

1e62124

lidizheng force-pushed the wait branch from 8a5f65e to 1e62124 Compare August 2, 2019 21:57

lidizheng added the kokoro:force-run label Aug 2, 2019

grpc-kokoro removed the kokoro:force-run label Aug 2, 2019

Apply the spin wait mechansim

3e84f47

lidizheng force-pushed the wait branch from aa10255 to 3e84f47 Compare August 2, 2019 22:38

lidizheng merged commit cd29d5d into grpc:master Aug 5, 2019

gnossen mentioned this pull request Aug 5, 2019

Quit waiting for a day #19852

Merged

gnossen mentioned this pull request Aug 20, 2019

Add a quick-start example for basic usages in Python #19938

Merged

william-plano-oxb mentioned this pull request Oct 23, 2019

add wrapper for grpc.Server.wait_for_termination opentracing-contrib/python-grpc#25

Open

lock bot locked as resolved and limited conversation to collaborators Nov 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wait_for_termination method to grpc.Server #19299

Add wait_for_termination method to grpc.Server #19299

lidizheng commented Jun 8, 2019 •

edited

Loading

gnossen left a comment

gnossen Jun 10, 2019

lidizheng Jun 12, 2019

gnossen left a comment

gnossen Jun 13, 2019

lidizheng Jun 13, 2019

gnossen Jun 13, 2019

lidizheng Jun 13, 2019

mehrdada commented Jun 13, 2019 •

edited

Loading

mehrdada Jun 13, 2019 •

edited

Loading

lidizheng Jun 13, 2019

mehrdada Jun 13, 2019

lidizheng Jun 13, 2019

lidizheng commented Jun 13, 2019

mehrdada left a comment

mehrdada Jun 13, 2019

lidizheng Jun 13, 2019

mehrdada Jun 13, 2019 •

edited

Loading

lidizheng Jun 13, 2019

mehrdada Jun 13, 2019

mehrdada Jun 13, 2019

lidizheng Jun 13, 2019

mehrdada left a comment

lidizheng commented Jun 13, 2019

mehrdada commented Jun 13, 2019

lidizheng commented Jun 13, 2019

mehrdada commented Jun 13, 2019

lidizheng commented Jun 14, 2019 •

edited

Loading

Add wait_for_termination method to grpc.Server #19299

Add wait_for_termination method to grpc.Server #19299

Conversation

lidizheng commented Jun 8, 2019 • edited Loading

gnossen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnossen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehrdada commented Jun 13, 2019 • edited Loading

mehrdada Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidizheng commented Jun 13, 2019

mehrdada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehrdada Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehrdada left a comment

Choose a reason for hiding this comment

lidizheng commented Jun 13, 2019

mehrdada commented Jun 13, 2019

lidizheng commented Jun 13, 2019

mehrdada commented Jun 13, 2019

lidizheng commented Jun 14, 2019 • edited Loading

lidizheng commented Jun 8, 2019 •

edited

Loading

mehrdada commented Jun 13, 2019 •

edited

Loading

mehrdada Jun 13, 2019 •

edited

Loading

mehrdada Jun 13, 2019 •

edited

Loading

lidizheng commented Jun 14, 2019 •

edited

Loading