Fix cancellation order between pipelines and tasks in CancelTasks#21642
Conversation
|
I think this fixes duckdb/duckdb-ui#12, reproduction it's not super tight, but point at this direction. Other potentially related issues that might be (or not) connected to this fix are: And, although it might be a stretch: |
There was a problem hiding this comment.
Thanks for the PR!
Actually looking at this I think this changeset is wrong - the current destruction order is correct. Note that everything that is cleared is vectors holding shared pointers (e.g. shared_ptr<Event>). We have weak_ptr<Pipeline> elsewhere that we use to check if pipelines / events have been destroyed.
I'm guessing the actual problem is that somewhere there's an Event & or Pipeline & without correct ownership.
Currently this seems to break interruption (as indicated by the timeouts in CI).
Edit: I stand correct - I was thinking of this but that is old code that has been removed since. This might indeed be the correct solution (and we should then probably move back to having pipelines be unique_ptr<Pipeline> that live in the executor).
CancelTasks was destroying pipelines/events/states while tasks still held references to them. Fix: set cancelled=true and drain all tasks first, then destroy pipelines.
d5eb4fa to
a80364e
Compare
a80364e to
c2ac5a9
Compare
|
Thanks - I think this is good to go. We might want to look at lifetimes of the tasks referencing pipelines when the executor is destroyed separately. |
Date: 2026-03-27 10:01:03 +0100 Fix cancellation order between pipelines and tasks in CancelTasks (duckdb/duckdb#21642) bump delta and unity_catalog ext refs in v1.5-variegata (duckdb/duckdb#21640)
Wrong order in CancelTasks I can't repro causing issues in current duckdb state, but it does become a problem by introducing more complex task logic, where I did found a bunch of problems that were tied to this.