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

stub.withWaitForReady()'s DEADLINE_EXECEEDED lacks connection failure information #10749

Open
mohit-0109 opened this issue Dec 14, 2023 · 4 comments · May be fixed by #11851
Open

stub.withWaitForReady()'s DEADLINE_EXECEEDED lacks connection failure information #10749

mohit-0109 opened this issue Dec 14, 2023 · 4 comments · May be fixed by #11851
Labels
enhancement
Milestone
Next

Comments

@mohit-0109
Copy link

mohit-0109 commented Dec 14, 2023 •
edited by ejona86
Loading 

try {
  stub.withWaitForReady().withDeadline().hello();
} catch(StatusRuntimeException e) {
  // always receiving DEADLINE_EXCEEDED as status code if server is unavailable or client certs are invalid
}

There is no way to distinguish the underlying cause of failure , say if the failure is due to SSLHanshake or server genuinely not being available.

As an alternative, without using withWaitForReady() we are receiving UNAVAILABLE code in StatusRuntimeException which wraps the underlying cause say for example the SSLHandshake exception. Looking at the UNAVAILABLE code how can we safely take a call if we to reattempt connection, as we do not want to reattempt the connection in SSLException scenarios?
@ejona86
Copy link
Member

ejona86 commented Dec 19, 2023

Drive-by comment: What needs to happen here is we enhance DelayedStream/PendingStream to remember the last pick failure and then add it during appendTimeoutInsight() if there is no real stream. We've been aware of this shortcoming, but most people aren't using waitForReady(). Then you'd get better error information without doing anything in particular (as long as DEADLINE_EXCEEDED still triggers). #6569 might be related.

@YifeiZhuang YifeiZhuang added this to the Next milestone Dec 20, 2023
@ejona86
Copy link
Member

ejona86 commented Jan 9, 2025

I don't think we'd provide any guarantees that you can programatically distinguish between the server can't be contacted and invalid client certs. In particular, I believe the exact error you get for invalid client certs even without waitForReady can vary depending on lots of details.

But for this we can make it so a human could know which case was being hit.

@ejona86 ejona86 changed the title stub.withWaitForReady().withDeadline is masking the underlying exceptions stub.withWaitForReady()'s DEADLINE_EXECEEDED lacks connection failure information Jan 9, 2025
@shivaspeaks
Copy link
Member

shivaspeaks commented Jan 21, 2025 •
edited
Loading

PendingStream to remember the last pick failure

By this you mean to remember shutdownStatus of PickerState? To do so, we should define a data member in DelayedStream.java and update this inside the PendingStream as it has access to PickerState.

And then adding it during appendTimeoutInsight() if there is no real stream in DelayedStream!

@ejona86
Copy link
Member

ejona86 commented Jan 21, 2025

No, not shutdownStatus. That is only used when the channel itself is shut down. The "last pick failure" would be in the value returned from pickSubchannel().

@shivaspeaks shivaspeaks linked a pull request Jan 23, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
Next
Development

Successfully merging a pull request may close this issue.

core: remember last pick status in no real stream shivaspeaks/grpc-java
4 participants
@ejona86 @YifeiZhuang @mohit-0109 @shivaspeaks