Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Examples: Updated HelloWorldServer using Executor #11850

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

NaveenPrasannaV
Copy link
Contributor

Updated HelloWorldServer Example using Executor

Fix: #2263

@shivaspeaks shivaspeaks added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jan 31, 2025
@grpc-kokoro 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 Jan 31, 2025
private void start() throws IOException {
/* The port on which the server should run */
int port = 50051;
server = Grpc.newServerBuilderForPort(port, InsecureServerCredentials.create())
server = ServerBuilder.forPort(port)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want people to use the Grpc.newServerBuilderForPort instead of ServerBuilder.forPort, so this change is wrong in addition to unnecessary. Both of the methods return a ServerBuilder<?>.

@@ -58,6 +63,7 @@ public void run() {
private void stop() throws InterruptedException {
if (server != null) {
server.shutdown().awaitTermination(30, TimeUnit.SECONDS);
executor.shutdown();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since executor is created by the constructor rather than start(), you should do the executor.shutdown() in the shutdown hook. It would go both after the stop() and inside the catch block. The catch block should also call server.shutdownNow()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update examples to use Executor
4 participants