sqlproxyccl: pass in ctx when transferring connections#154266
sqlproxyccl: pass in ctx when transferring connections#154266craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
|
Regarding the Blathers testing suggestion, it's possible there's some issue with the test itself that is causing the flake linked in the "Fixes" ticket, in which case we'll find out with future tickets. But at the very least this PR fixes a panic caused by an actual mismanagement of spans. |
| } | ||
| } | ||
|
|
||
| func newTransferContext(backgroundCtx context.Context) (*transferContext, context.CancelFunc) { |
There was a problem hiding this comment.
This is subtle, but I don't think this a complete fix. It's possible the forwarder's span was already free'd by the time we are calling newTransferContext.
I think fixing this requires us to attach a span to the forwarder, or add a context as an argument to TransferConnection and use that context instead of the forwarder's context.
Passing a context with a span into TransferConnection feels more idiomatic. I'm not sure why this code was using the forwarder's context.
There was a problem hiding this comment.
PTAL. I don't believe that I need to fork the span anymore in newTransferContext, but let me know if I'm misunderstanding.
There was a problem hiding this comment.
This change looks good to me. I'll approve it once CI is green.
ca1a7a7 to
18a7e08
Compare
efffc43 to
cd354d2
Compare
|
There is a static check failure: |
| // | ||
| // TransferConnection implements the balancer.ConnectionHandle interface. | ||
| func (f *forwarder) TransferConnection() (retErr error) { | ||
| func (f *forwarder) TransferConnection(transferCtx context.Context) (retErr error) { |
There was a problem hiding this comment.
nit: I think you can call this parameter ctx. Since it is used as a conventional context.
There was a problem hiding this comment.
Sounds good. I had to rename another variable to transferCtx
Previously, the span used when transferring connections was reused from the forwarder. However, transferring connections has async components, so it's possible that the forwarder calls `Finish` on the span before the connection transfer is done using the span. This leads to a panic: `panic: use of Span after Finish`. To address this, this patch passes a context in when transferring a connection, instead of using the forwarder's context. Fixes: cockroachdb#153569 Release note: None
cd354d2 to
65d0b22
Compare
davidwding
left a comment
There was a problem hiding this comment.
Let's see if I fixed the test failure too...I wasn't handling the context objects in the test properly 🤞
| // | ||
| // TransferConnection implements the balancer.ConnectionHandle interface. | ||
| func (f *forwarder) TransferConnection() (retErr error) { | ||
| func (f *forwarder) TransferConnection(transferCtx context.Context) (retErr error) { |
There was a problem hiding this comment.
Sounds good. I had to rename another variable to transferCtx
|
TFTR! |
|
bors r=jeffswenson |
|
Build succeeded: |
Previously, the span used when transferring connections was reused from
the forwarder. However, transferring connections has async components,
so it's possible that the forwarder calls
Finishon the span beforethe connection transfer is done using the span. This leads to a panic:
panic: use of Span after Finish. To address this, this patch passes acontext in when transferring a connection, instead of using the
forwarder's context.
Fixes: #153569
Release note: None