client: improve connection errors from RPCs and Dial WithReturnConnectionError #4190
Conversation
…s closed, so we don't return generic error
| @@ -1309,6 +1312,7 @@ func (t *http2Client) reader() { | |||
| // Check the validity of server preface. | |||
| frame, err := t.framer.fr.ReadFrame() | |||
| if err != nil { | |||
| t.updateConnectionError(err) | |||
There was a problem hiding this comment.
One thing about my design in #4163 that I preferred is that adding an error to Close means we're forced to inspect every place it is called (now and in the future) so we can improve our error reporting in all instances where the transport is closed. With this alternate implementation, we only optionally improve it. I'm fine with the LastConnectionError() method (though it is stateful, and, as such, I do prefer the alternative of making OnClose accept an error result). But we should really make http2Client.Close accept an error. If there is no useful error at the site where Close is called, just pass ErrConnClosing.
If we do that, we also know that whenever OnClose is called, there must be a non-nil error, and we can simplify the clientconn.go code to always return nil, nil, t.LastConnectionError() (without the nil check).
There was a problem hiding this comment.
Ok, I will go with your idea. Should I keep LastConnectionError or have onClose callback to receive the connection error as parameter?
There was a problem hiding this comment.
LastConnectionError is stateful, meaning it requires synchronization and opens the possibility for a new class of bugs. I think if OnClose takes an error to pass the information, it makes for a better flow. (This is "share memory by communicating instead of communicating by sharing memory," it just doesn't require a channel.)
|
|
||
| c.validCert, err = c.generateCert(ca, time.Now().Add(time.Hour)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to generate valid cert: %v", err) |
There was a problem hiding this comment.
You should be able to simplify things significantly by just panicking whenever an error happens inside generateCert. Failing "gracefully" for something we expect to pass, during initialization, isn't important.
| clientCerts := clientCertificates{} | ||
| if err := clientCerts.generateCertificates(&ca); err != nil { |
There was a problem hiding this comment.
Combine these into clientCerts, err := newClientCertificates(&ca)?
| creds := credentials.NewTLS(cfg) | ||
|
|
||
| dialCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| cc, err := grpc.DialContext( |
There was a problem hiding this comment.
Can you please also add a check for RPC errors:
cc, err := grpc.Dial(.../* no WithBlock or WithReturnConnectionError*/)
...
client := testpb.NewTestServiceClient(cc)
res, err := client.EmptyCall(...)
// Ensure err contains the connection errorYou should be able to use the stubserver.StubServer to make it easy to create the client and server (server and dial options can be provided to the Start method); check the other tests for examples.
dfawley
changed the title
|
This issue is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
This is the implementation regarding issue-4163. I didn't follow the instructions, because it involved a lot more code changes. Implementation tries to copy the same approach used in ClientConn error saving.
Fixes #4163