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

surface original network errors and create error stacks #425

Merged
merged 1 commit into from
Sep 19, 2019

Conversation

gleonid
Copy link
Contributor

@gleonid gleonid commented Sep 9, 2019

In terms of priorities this is a very minor change.

in some cases, like packet/conn.go:83, root cause error is being lost
surfacing original errors is useful for troubleshoot production issues

@@ -80,7 +80,7 @@ func (c *Conn) ReadPacketTo(w io.Writer) error {
header := []byte{0, 0, 0, 0}

if _, err := io.ReadFull(c.Conn, header); err != nil {
return ErrBadConn
return errors.Wrapf(ErrBadConn, "io.ReadFull(header) failed. err %v", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

preserving dummy ErrBadConn here, because code in canal/canal.go, failover/server.go and driver/driver.go is checking for it by applying errors.Cause

@@ -177,17 +177,19 @@ func (c *Conn) WritePublicKeyAuthPacket(password string, cipher []byte) error {
// request public key
data := make([]byte, 4+1)
data[4] = 2 // cachingSha2PasswordRequestPublicKey
c.WritePacket(data)
if err := c.WritePacket(data); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a liberty to add error checking here

@siddontang
Copy link
Collaborator

PTAL @GregoryIan

can this break the compatibility? if we check ErrBadConn error outside directly?

@IANTHEREAL
Copy link
Collaborator

Cool Job!Please resolve conflicts @gleonid

@IANTHEREAL
Copy link
Collaborator

IANTHEREAL commented Sep 17, 2019

@siddontang If choosing, I expect the client to use errors.Cause for specification error handling. What about you?

@siddontang
Copy link
Collaborator

Is it necessary to wrap all the errors? or just handling some specific errors is fine?

@gleonid
Copy link
Contributor Author

gleonid commented Sep 17, 2019

Is it necessary to wrap all the errors? or just handling some specific errors is fine?

Clearly it is not necessary to neither wrap, nor check (line 180) all errors, as it functions fine without any of that.
I personally believe that good and detailed error reporting is a must for production systems, with that in mind, I prefer to have all errors wrapped, so on error you'll have a "call stack" in your logs, regardless of which logging package you use, which I found to be invaluable.

@gleonid
Copy link
Contributor Author

gleonid commented Sep 17, 2019

Cool Job!Please resolve conflicts @gleonid

done

@siddontang siddontang merged commit 87265c0 into go-mysql-org:master Sep 19, 2019
@gleonid gleonid deleted the improve_net_error_reporting branch October 17, 2019 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants