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

Use a new ConnectionError type for network errors #418

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dagss
Copy link
Contributor

@dagss dagss commented Sep 14, 2018

It has been useful for us to be able to test if returned errors are
variations of "network connection gone", so we have added this change to
our own code. Submitting it to upstream in case it is useful.

It has been useful for us to be able to test if returned errors are
variations of "network connection gone", so we have added this change to
our own code. Submitting it to upstream in case it is useful.
@denisenkom
Copy link
Owner

This looks like some kind of debugging improvement which probably should not change public interface. Maybe you should use logging to achieve the same goal?

@dagss
Copy link
Contributor Author

dagss commented Sep 8, 2019

No, there is definitely a need for applications to automatically react to conditions like this. Unless you are suggesting we hook the logger up to the caller and the caller parses it ...

@dagss
Copy link
Contributor Author

dagss commented Sep 8, 2019

I would argue the public interface is changed in a minor way? The only observable difference is which structs implements Error interface, but it looks the same from using Error?

@denisenkom
Copy link
Owner

Once this change introduced it becomes something that cannot be changed in non backward-compatible way. Also there is a good chance that users would like to distinguish other kinds of errors, like in Python DBAPI there is a rich class hierarchy for database errors.
Also you seems to be reporting network error only for writing operations but not for reading, which would be needed for this change to be consistent.

You said you needed this functionality, can you provide details into your use-case?

@dagss
Copy link
Contributor Author

dagss commented Sep 10, 2019

We simply want to retry the "outer" HTTP request into our backend automatically (without returning 503 and have the client do retry) for any kind of transient error.

We use it in one place, which also classifies a number of other errors as transient errors. So I guess we could return net.Error or a TransientError or similar.

Reporting only for writing and not for reading is probably a glitch / bug in this PR (unless those causes net.Error).

func IsTransientMssqlError(e error) bool {
	// See https://docs.microsoft.com/en-us/azure/sql-database/sql-database-develop-error-messages
	transientMssqlErrors := []int32{
		4060, 40197, 40501, 40613, 49918, 49919, 49920, 4221,
	}
	if cause, ok := errors.Cause(e).(mssql.Error); ok {
		num := cause.Number
		for _, ec := range transientMssqlErrors {
			if ec == num {
				return true
			}
		}
	}
	return false
}

func IsTransientError(e error) bool {
	cause := errors.Cause(e)

	if IsTransientMssqlError(cause) {
		return true
	}

	if cause == driver.ErrBadConn {
		return true
	}

	switch cause.(type) {
	case mssql.ConnectionError:
		return true
	case mssql.StreamError:
		return true
	case net.Error:
		return true
	}

	return false
}

@serbrech
Copy link

This seems to be a consequence of the driver not returning ErrBadConn.
if the driver was returning ErrBadConn, the database/sql package would automatically get rid of the connection from the pool. see #586

I don't think we should go deeper into a custom error implementation, getting the driver further away from the protocol it is suppose to implement.

If we move to go 1.15, the Validator interface would allow this kind of special error handling though.

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.

None yet

3 participants