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

add dealine to close main server info fetch stream for unary rpcs. #146

Closed
wants to merge 1 commit into from
Closed

add dealine to close main server info fetch stream for unary rpcs. #146

wants to merge 1 commit into from

Conversation

abb3r
Copy link

@abb3r abb3r commented Nov 14, 2023 •
edited
Loading

Signed-off-by: Rahman Abber Tahir [email protected]

@abb3r abb3r force-pushed the circumventMainStreamCloseHang branch from 31f87fd to 5e982e1 Compare November 14, 2023 12:09
@abb3r
Copy link
Author

abb3r commented Nov 14, 2023 •
edited
Loading

Now when we have such hangs we would see:

gwhisper --rpcTimeoutMilliseconds=2000 127.0.0.1 examples.StatusHandling rpcSleepForSeconds number=5

RPC failed ;( Status code: 4 DEADLINE_EXCEEDED, error message: Deadline Exceeded
Note: You can increase the deadline by setting the --rpcTimeoutMilliseconds option to a number or 'None'.
ServerReflectionInfo rpc failed. Grpc Server failed to close the stream within 10 seconds.
Error code: 4, message: Deadline Exceeded, debug info: {"created":"@1699964194.163491587","description":"Error received from peer ipv4:127.0.0.1:50051","file":"/home/rahman/repos/os-gwhisper/gWhisper/build/_deps/grpc-src/src/core/lib/surface/call.cc","file_line":1075,"grpc_message":"Deadline Exceeded","grpc_status":4}

@abb3r
Copy link
Author

abb3r commented Nov 14, 2023 •
edited
Loading

Hi @rainerschoe, as we discussed about the 'gwhisper hangs when server fails to close the reflection stream'
I think this change would be a good enough circumvention since we only find issue with unary rpcs and they always have a default timeout of 10seconds. It can provide a safety net and ease up test scenarios until there is a change for a more dynamic timeout setting, what do you say?

@abb3r abb3r closed this Nov 15, 2023
rainerschoe
rainerschoe reviewed Nov 15, 2023
View reviewed changes
Copy link
Member

@rainerschoe rainerschoe left a comment

Choose a reason for hiding this comment

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

Thanks for the PR (even if it is closed now). See my comment on your default timeout. This could actually work, when we close the reflection stream early enough.

third_party/gRPC_utils/proto_reflection_descriptor_database.cc
@@ -315,7 +326,11 @@ void ProtoReflectionDescriptorDatabase::AddFileFromResponse(

const std::shared_ptr<ProtoReflectionDescriptorDatabase::ClientStream>
ProtoReflectionDescriptorDatabase::GetStream() {
if (!stream_) {
if (!stream_) //only assign deadline to a unary rpc.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not the place you are looking for.
It is kind of a stream singleton constructor. if stream does not exist it is creating it.
reflection is always a stream.

third_party/gRPC_utils/proto_reflection_descriptor_database.cc
@@ -31,6 +31,7 @@ using grpc::reflection::v1alpha::ServerReflection;
using grpc::reflection::v1alpha::ServerReflectionRequest;
using grpc::reflection::v1alpha::ServerReflectionResponse;

const uint8_t g_timeoutGrpcMainStreamSeconds = 10; //using default gwhisper timeout of 10 seconds.
Copy link
Member

Choose a reason for hiding this comment

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

I have a new idea:
gWhisper uses reflection only BEFORE running the actual user-requested RPC:

  1. gWhisper starts parsing CLI arguments until hostname
  2. once hostname is known, reflection stream is established with the server and via multiple requests all the RPC names and type information is fetched from the reflection endpoint while CLI args are parsed and verified against this.
  3. The parsed CLI args are converted into a protobuf message
  4. The actual RPC is called as requested with the user (may be streaming or unary RPC)
  5. response is formatted and printed

I would need to verify, but I guess that currently, we keep the reflection stream open for the whole duration. which causes problems when using a fixed reflection deadline.

What we could do is close the reflection stream after 2 or 3. This way we could set a fixed reflection stream RPC timeout and we would not influence 4, even if the user executes a very long-running RPC.

Copy link
Author

@abb3r abb3r Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @rainerschoe thanks for the review. I did see some deficiencies via functional tests and I like your idea of closing the desc stream early.

Hence, in the new PR #147

  • Dynamic deadlines are now propagated to the DescDb stream.
  • The db stream stream is now closed after 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rainerschoe rainerschoe rainerschoe left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@abb3r @rainerschoe