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.

Sign up for GitHub

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

Jump to bottom

Modify udp_server to postpone shutdown_fd() during shutdown. #10440

Merged
merged 2 commits into from
Apr 14, 2017
Merged

Modify udp_server to postpone shutdown_fd() during shutdown. #10440

merged 2 commits into from
Apr 14, 2017

Conversation

danzh2010
Copy link
Contributor

Since udp_server only has one fd, it shouldn't be shutdown immediately in grpc_udp_server_destroy() when there are in-flight requests. Instead, server should wait till all requests finishes to shut down the fd. This cl add a closure with a callback of shutdowning fd to grpc_udp_listener. This callback is passed through orphan_cb() in grpc_udp_listener to notify transport layer that fd is about to be orphaned and trigger the callback to shutdown fd when transport layer thinks it's the time to do so.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

13 similar comments
@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@mpwarres mpwarres requested a review from yang-g April 3, 2017 15:27
@danzh2010
Copy link
Contributor Author

ping?

@yang-g
Copy link
Member

yang-g commented Apr 4, 2017

jenkins: this is ok to test

@yang-g
Copy link
Member

yang-g commented Apr 4, 2017

@danzh2010 Please squash commits. Thanks.

@grpc-kokoro
Copy link 

No significant performance differences

@danzh2010
Copy link
Contributor Author

done. Thanks @yang-g

@grpc-kokoro
Copy link 

No significant performance differences

@yang-g
Copy link
Member

yang-g commented Apr 6, 2017

clang-format is not happy:

  • tools/dockerfile/grpc_clang_format/clang_format_all_the_things.sh
    --- /var/local/git/grpc/src/core/lib/iomgr/udp_server.h 2017-04-05 16:24:47.586392809 +0000
    +++ /tmp/tmp.wtwNoO0coE 2017-04-05 16:24:58.982389824 +0000
    @@ -56,7 +56,7 @@
    /* Called when the grpc_fd is about to be orphaned (and the FD closed). */
    typedef void (*grpc_udp_server_orphan_cb)(grpc_exec_ctx *exec_ctx,
    grpc_fd *emfd,
  •                                      grpc_closure* shutdown_fd_callback,

  •                                      grpc_closure *shutdown_fd_callback,
                                     void *user_data);

/* Create a server, initially not bound to any ports */
--- /var/local/git/grpc/test/core/iomgr/udp_server_test.c 2017-04-05 16:24:47.926392720 +0000
+++ /tmp/tmp.u4Qdj7Y1pF 2017-04-05 16:25:11.582386523 +0000
@@ -91,7 +91,7 @@
}

static void on_fd_orphaned(grpc_exec_ctx *exec_ctx, grpc_fd *emfd,

  •                       grpc_closure* closure, void *user_data) {

  •                       grpc_closure *closure, void *user_data) {
    gpr_log(GPR_INFO, "gRPC FD about to be orphaned: %d",
    grpc_fd_wrapped_fd(emfd));
    g_number_of_orphan_calls++;

@danzh2010
Copy link
Contributor Author

PTAL

@yang-g
Copy link
Member

yang-g commented Apr 6, 2017

LGTM

Let's wait for the tests. Thanks.

@grpc-kokoro
Copy link 

No significant performance differences

@yang-g
Copy link
Member

yang-g commented Apr 14, 2017

Test failure: #9562

@yang-g yang-g merged commit 52ff44f into grpc:master Apr 14, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yang-g yang-g yang-g approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@danzh2010 @grpc-kokoro @yang-g @googlebot @danzh1989