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

[call-v3] Client call implementation #36724

Closed
wants to merge 723 commits into from
Closed

Conversation

ctiller
Copy link
Member

@ctiller ctiller commented May 24, 2024

No description provided.

Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This looks really good! Comments are basically what we discussed.

src/core/lib/surface/call_utils.h Outdated Show resolved Hide resolved
src/core/lib/surface/client_call.h Outdated Show resolved Hide resolved
src/core/lib/surface/client_call.cc Outdated Show resolved Hide resolved
src/core/lib/surface/client_call.cc Outdated Show resolved Hide resolved
src/core/client_channel/client_channel.h Outdated Show resolved Hide resolved
src/core/ext/transport/chaotic_good/server_transport.cc Outdated Show resolved Hide resolved
src/core/server/server.cc Outdated Show resolved Hide resolved
test/core/surface/lame_client_test.cc Show resolved Hide resolved
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Looks great!

src/core/client_channel/client_channel.h Outdated Show resolved Hide resolved
@copybara-service copybara-service bot closed this in 90a649f Jun 5, 2024
copybara-service bot pushed a commit that referenced this pull request Jun 13, 2024
…ls and process shutdown (#36903)

Fixes the CBF of `src/ruby/end2end/killed_client_thread_test.rb` (failure mode is a hang of the child process that receives the SIGTERM) that has been happening since #36724

So far grpc-ruby CQ pluck operations have so far used a 20ms-interval busy poll to check interrupts in case we've received a signal, handle process shutdown, etc. This means ongoing RPCs will not terminate their CQ operations if we need to terminate the process (the loop simply exits without waiting for the CQ op to finish), causing a leak. Those RPCs can leave refs over their corresponding channels preventing [this](https://github.com/grpc/grpc/blob/8564f72e8e0334c25c480e0aec1df75bdc1fce14/src/ruby/ext/grpc/rb_channel.c#L653) from terminating (the channels don't reach state SHUTDOWN after being destroyed).

Fix is to unblock CQ pluck operations by cancelling calls, and thus allowing the CQ pluck to actually complete its operation. For server listening CQ operations, we unblock them by shutting down the server.

A side win here is to remove the [20ms-interval busy poll](https://github.com/grpc/grpc/blob/8564f72e8e0334c25c480e0aec1df75bdc1fce14/src/ruby/ext/grpc/rb_completion_queue.c#L44) on CQ operations, which was only needed to handle shutdown.

Closes #36903

COPYBARA_INTEGRATE_REVIEW=#36903 from apolcyn:fix_ruby_interrupt bed1ee2
PiperOrigin-RevId: 643046465
apolcyn added a commit to apolcyn/grpc that referenced this pull request Jun 13, 2024
…ls and process shutdown (grpc#36903)

Fixes the CBF of `src/ruby/end2end/killed_client_thread_test.rb` (failure mode is a hang of the child process that receives the SIGTERM) that has been happening since grpc#36724

So far grpc-ruby CQ pluck operations have so far used a 20ms-interval busy poll to check interrupts in case we've received a signal, handle process shutdown, etc. This means ongoing RPCs will not terminate their CQ operations if we need to terminate the process (the loop simply exits without waiting for the CQ op to finish), causing a leak. Those RPCs can leave refs over their corresponding channels preventing [this](https://github.com/grpc/grpc/blob/8564f72e8e0334c25c480e0aec1df75bdc1fce14/src/ruby/ext/grpc/rb_channel.c#L653) from terminating (the channels don't reach state SHUTDOWN after being destroyed).

Fix is to unblock CQ pluck operations by cancelling calls, and thus allowing the CQ pluck to actually complete its operation. For server listening CQ operations, we unblock them by shutting down the server.

A side win here is to remove the [20ms-interval busy poll](https://github.com/grpc/grpc/blob/8564f72e8e0334c25c480e0aec1df75bdc1fce14/src/ruby/ext/grpc/rb_completion_queue.c#L44) on CQ operations, which was only needed to handle shutdown.

Closes grpc#36903

COPYBARA_INTEGRATE_REVIEW=grpc#36903 from apolcyn:fix_ruby_interrupt bed1ee2
PiperOrigin-RevId: 643046465
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants