Return useful error for busy connections - #1787
Conversation
Walkthrough
ChangesBusy buffer prepare handling
Author list update
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
How did you verify that there are no cases where ErrBusyBuffer is returned for other reasons? |
|
You're right — I had not verified that assumption globally. I traced all ErrBusyBuffer producers. It is returned by takeBuffer, takeSmallBuffer, and takeCompleteBuffer whenever the shared read buffer is non-empty, and those helpers are also used by authentication and other write paths. Therefore, the previous global error message was too specific. I pushed 4cbd816 to restore the original ErrBusyBuffer message and add cause-neutral context only in Prepare: The Prepare path is writeCommandPacketStr -> takeBuffer, so the added context reflects the verified buffer invariant without assuming that an open result set is the only possible cause. errors.Is(err, ErrBusyBuffer) is still preserved. |
|
First of all, I don't think it's a good idea to make only the Prepare error messages user-friendly. What about Exec? Query? And I also need to consider whether error wrapping is necessary by checking all the occurrence patterns of ErrBusyBuffer. If we can solve it by just improving the basic message of ErrBusyBuffer a little, we should do that. However, I am not going to investigate, think about, or make a decision on that right now. This issue is very low priority for me. |
|
Thanks for the clarification. I understand that improving only the Prepare path would be inconsistent if ErrBusyBuffer can also surface through Exec, Query, and other paths. I will avoid pushing further changes until the preferred scope is clearer. If helpful, I would be happy to audit all ErrBusyBuffer call paths and summarize the possible behaviors and trade-offs. |
Description
When a previous result set still owns the connection buffer,
Preparelogsbusy bufferand converts the error todriver.ErrBadConn. This hides the actual caller mistake.This change:
ErrBusyBufferfromPrepareinstead ofdriver.ErrBadConnAUTHORSFixes #526.
Checklist
Validation
go test ./... -run 'TestPrepareBusyBuffer|TestPrepareWithOpenRows' -count=1go vet ./...