sql: do not close stmt buffer of internal executor in errCallback #80070
Conversation
|
This change is |
There was a problem hiding this comment.
This seems fine but I don't feel qualified to sign off on it, can you get another reviewer more familiar with this code?
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)
|
Unfortunately, I don't think anyone on our team has any familiarity with this code. @jordanlewis was the main reviewer of #59330 when the code in question was introduced, so maybe you can take a quick look? |
|
@mgartner I think you mentioned that you need to get familiar with connExecutor for UDFs, so maybe could take a look at this too? |
|
@yuzefovich, is there a way we can test this scenario you describe? It sounds like an interesting edge case, yet clearly it's not tested at all right now @RichardJCai, aren't you working on internal executor soon? Could you take a look at this (and maybe talk more with Yahor about the change)? |
There was a problem hiding this comment.
Alright, I added a WIP commit with the test that follows exactly the scenario I described in the commit message. Everything behaves exactly as I described. However, without introducing several knobs I don't know how to make such a test non-flaky, and adding the knobs seems not worth it just for this edge case, so I'm inclined to not merge the test.
Reviewable status:
There was a problem hiding this comment.
nit: fix "the new goroutine is span..." in the PR description
Reviewed 1 of 1 files at r1, 3 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:
-- commits line 17 at r2:
nit: formatting typo here?
Previously, we would close the stmt buffer of the internal executor in `errCallback`, "just to be safe" since it was assumed that the buffer is already closed when the callback is executed. The callback runs whenever `run()` loop of connExecutor exits with an error. However, it is possible for the following sequence of events to happen: - The new goroutine is spun up for the internal executor before any commands are pushed into the stmt buffer. - The context is canceled before the new goroutine blocks waiting for the command to execute, i.e. `run()` loop is exited before any commands are executed. - The `errCallback` with the context cancellation error is evaluated. This closes the stmt buffer. The goroutine exits. - The main goroutine tries to push some commands into the buffer only to find that it was already closed. An assertion error is returned, and a sentry event is created. I think we should just not close the stmt buffer in the `errCallback` since this was never necessary and can lead to the scenario described above where no sentry event should be emitted. Release note: None
yuzefovich
added
backport-22.1.x
and removed
do-not-merge
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
|
Build succeeded: |
Previously, we would close the stmt buffer of the internal executor in
errCallback, "just to be safe" since it was assumed that the buffer isalready closed when the callback is executed. The callback runs whenever
run()loop of connExecutor exits with an error.However, it is possible for the following sequence of events to happen:
commands are pushed into the stmt buffer.
the command to execute, i.e.
run()loop is exited before any commandsare executed.
errCallbackwith the context cancellation error is evaluated.This closes the stmt buffer. The goroutine exits.
find that it was already closed. An assertion error is returned, and
a sentry event is created.
I think we should just not close the stmt buffer in the
errCallbacksince this was never necessary and can lead to the scenario described
above where no sentry event should be emitted.
Fixes: #79746.
Release note: None