Gracefully shutdown example servers by dounan · Pull Request #6512 · grpc/grpc-java

dounan · 2019-12-11T05:37:39Z

The example servers do not properly wait until preexisting calls complete before shutting down (#6511).

This PR

Adds a call to awaitTermination to wait for preexisting calls to complete (with a 30s timeout)
Updates the API documentation for Server.shutdown to clarify that the call does not wait for preexisting calls to complete before returning.

…alls

linux-foundation-easycla · 2019-12-11T05:37:42Z

The committers are authorized under a signed CLA.

✅ Dounan Shi (5cb0f94, 6784bca)

ejona86

Hmmm... something else is going on. The blockUntilShutdown() in the main() should be enough...

ejona86 · 2019-12-11T18:46:43Z

@@ -104,8 +104,8 @@ public List<ServerServiceDefinition> getMutableServices() {

  /**
   * Initiates an orderly shutdown in which preexisting calls continue but new calls are rejected.


I think "initiates" is the key word here.

I think this would probably go better as a second paragraph that is merely informative. Something like:

Note that this method does not wait for the server to stop, so existing calls may continue after returning. To stop existing calls use {@link #shutdownNow()} and to wait for the server to fully stop use {@link #awaitTermination()}.

Combined your and @sanjaypujare's suggestions: af9a157

sanjaypujare · 2019-12-11T18:49:31Z

-   * After this call returns, this server has released the listening socket(s) and may be reused by
-   * another server.
+   * This call will not wait for preexisting calls finish before returning. After this call returns,
+   * this server has released the listening socket(s) and may be reused by another server.


I suggest the following:

Initiates an orderly shutdown to allow preexisting calls to continue but new calls are
rejected. {@link #awaitTermination()} or {@link #awaitTermination(long, TimeUnit)} needs
to be called to wait for existing calls to finish. After this call returns, this server
has released the listening socket(s) and may be reused by another server.

Combined your and @ejona86's suggestions: af9a157

dounan · 2019-12-11T18:51:00Z

Thanks for taking a look @ejona86!

Hmmm... something else is going on. The blockUntilShutdown() in the main() should be enough...

From my understanding, the blockUntilShutdown() is useful to keep the Java application running so that the gRPC can serve requests, which is a bit different than handling the JVM's shutdown process.

When the JVM is instructed to shutdown (such as via a SIGINT), it will call all registered shutdown handlers that are registered via addShutdownHook:

Runtime.getRuntime().addShutdownHook(new Thread() {
    @Override
    public void run() {
        ...
    }
});

Once all those run methods return, the JVM will shutdown.

For this reason, it is important for the shutdown hook's run() method to not return until you are ok with the JVM shutting down, hence the additional call to server.awaitTermination()

sanjaypujare · 2019-12-11T18:52:50Z

Hmmm... something else is going on. The blockUntilShutdown() in the main() should be enough...

When you press control-C, it invokes the shutdown-hook. So blockUntilShutdown() in the main() is skipped right? In the original issue @dounan mentioned pressing control-C.

ejona86 · 2019-12-11T18:56:28Z

Once all those run methods return, the JVM will shutdown.

That's not true. On ctrl-c, the JVM simply calls start() on the threads registered, and nothing really more. What causes JVM shutdown is when there are no non-daemon threads. The main thread is non-daemon, so it should keep the JVM alive.

ejona86 · 2019-12-11T18:56:52Z

I'm going to move the conversation back to issue #6511.

ejona86

The new documentation looks good to me. I want to hold off on adding the awaitTerminations until we gain a bit better understanding what is going on.

sanjaypujare · 2019-12-11T19:17:28Z

+   * After this call returns, this server has released the listening socket(s) and may be reused by
+   * another server.
+   *
+   * Note that this method will not wait for preexisting calls finish before returning.


preexisting calls finish => preexisting calls to finish

Doh! Good catch

ejona86

Okay. So I was wrong, we do need the hook to wait until the server is done.

ejona86

One small thing, otherwise looks good. Thank you!

sanjaypujare · 2019-12-12T17:21:46Z

+        try {
+          CompressingHelloWorldServerAllMethods.this.stop();
+        } catch (InterruptedException e) {
+          e.printStackTrace();


Couple of comments:

do you want to print to stderr in keeping with lines 65 and 71?
e.printStackTrace(System.err)

do you want to mention/comment saying this is because the logger is closed as part of shutdown?

and of course make this change everywhere.

do you want to print to stderr in keeping with lines 65 and 71?
e.printStackTrace(System.err)

Done in d997e5b

I also went ahead and fixed the call to logger in ManualFlowControlServer: 1df260a

do you want to mention/comment saying this is because the logger is closed as part of shutdown?

The call to System.err.println above already has this comment 😄

sanjaypujare

I have suggested a couple of changes.

sanjaypujare

LGTM

dounan added 2 commits December 10, 2019 21:34

examples: update example servers to perform proper graceful shutdown

5cb0f94

api: clarify that Server.shutdown() does not block on preexisting c…

6784bca

…alls

dounan mentioned this pull request Dec 11, 2019

Example HelloWorldServer does not gracefully shutdown #6511

Closed

ejona86 reviewed Dec 11, 2019

View reviewed changes

sanjaypujare reviewed Dec 11, 2019

View reviewed changes

ejona86 reviewed Dec 11, 2019

View reviewed changes

sanjaypujare reviewed Dec 11, 2019

View reviewed changes

dounan force-pushed the gracefully-shutdown-example-servers branch from af9a157 to 3328bfc Compare December 11, 2019 19:23

ejona86 reviewed Dec 11, 2019

View reviewed changes

Update shutdown API documentation

7bd69d3

dounan force-pushed the gracefully-shutdown-example-servers branch from 3328bfc to 7bd69d3 Compare December 11, 2019 19:57

dounan added 3 commits December 11, 2019 12:08

Update InterruptedException handling during shutdown

8e75b2d

Fix tests

53b54df

Fix checkstyle

5118c7b

ejona86 approved these changes Dec 11, 2019

View reviewed changes

Comment thread examples/src/test/java/io/grpc/examples/routeguide/RouteGuideServerTest.java Outdated

Replace tabs with spaces

91391d2

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 11, 2019

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 11, 2019

ejona86 requested a review from sanjaypujare December 11, 2019 23:05

sanjaypujare reviewed Dec 12, 2019

View reviewed changes

Print stacktrace to stderr

d997e5b

sanjaypujare approved these changes Dec 12, 2019

View reviewed changes

Fix ManualFlowControlServer logging during shutdown

1df260a

sanjaypujare added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 13, 2019

grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 13, 2019

sanjaypujare merged commit 9e02cf0 into grpc:master Dec 14, 2019

ejona86 mentioned this pull request Dec 14, 2019

examples: Throw exception in test instead of printStackTrace #6521

Merged

dounan mentioned this pull request Dec 17, 2019

Proper graceful shutdown dounan/grpc-spring-boot-starter#1

Closed

lock Bot locked as resolved and limited conversation to collaborators Mar 17, 2020

		@@ -104,8 +104,8 @@ public List<ServerServiceDefinition> getMutableServices() {

		/**
		* Initiates an orderly shutdown in which preexisting calls continue but new calls are rejected.

Conversation

dounan commented Dec 11, 2019

Uh oh!

linux-foundation-easycla Bot commented Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

ejona86 Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

dounan Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

sanjaypujare Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

dounan Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

dounan commented Dec 11, 2019

Uh oh!

sanjaypujare commented Dec 11, 2019

Uh oh!

ejona86 commented Dec 11, 2019

Uh oh!

ejona86 commented Dec 11, 2019

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

sanjaypujare Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

dounan Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sanjaypujare Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

dounan Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanjaypujare left a comment

Choose a reason for hiding this comment

Uh oh!

sanjaypujare left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linux-foundation-easycla Bot commented Dec 11, 2019 •

edited

Loading

dounan Dec 12, 2019 •

edited

Loading