-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Return ErrBusyBuffer instead of driver.ErrBadConn #611
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
Conversation
Interesting, we were always looking for a way to reproduce those errors.
In that case, we should probably return a more descriptive error like e.g. ErrUnreadData or more specific ErrUnreadTxRows. ErrBusyBuffer was just the best name we could come up for an error we couldn't find any explanation for.
And please don't ignore our pull request template, specifically Added myself / the copyright holder to the AUTHORS file in the checklist 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not return the error here. Leave the buffer unchanged.
This specific error should only be returned when we know that it is this specific error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example here it should be impossible that we're in a transaction with unread rows. Here the driver.ErrBadConn should still be returned as there is this is a brand new connection and there really shouldn't be any unread data in the buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had some time to sort things out, I think that's correct behaviour to return ErrBusyBuffer because when we cannot take it then something must be wrong in the code (multiple functions are using the buffer at the same time) or in this case buffer has some unread data.
And only two functions writePacket and readPacket should return ErrBadConn since they do the io.
So we should return ErrBusyBuffer anyway not ErrBadConn, and probably add additional checks to Query and Exec to return ErrUnreadRows to be more descriptive.
What do you think?
@amenzhinsky do you still have problems with the now implemented behavior (see #302)?
If yes, please update this PR or otherwise close it.
Close this because no reply.
Uh oh!
There was an error while loading. Please reload this page.
#314
We ran into problem when using the same tx and querying multiple times, the mysql driver returns
driver.ErrBadConnsending thebusy buffererror to stderr. I believe that in most cases buffer cannot be taken only when previous one hasn't been read out or closed.See
TestUnclosedRowsas an example in the changes.This is more like error message improvement since current behaviour is quite confusing.