Fix child QueryResult lifetime after parent close#1
Fix child QueryResult lifetime after parent close#1magyargergo wants to merge 3 commits intoLadybugDB:mainfrom
Conversation
|
Thank you! This is along the same lines as the recent fix on go-ladybug: While this PR improves things, we need a more comprehensive review of how the Do you prefer to get this in first and then discuss the rest of the issues in the comment below? |
|
Here's a potential ownership model:
Perhaps the right thing to do is to get this PR in and then work on removing non-owned references. Is that something you want to take on? |
|
Thank you for looking into this! I'll tackle with this today and get back to you. |
|
I took the stronger ownership-model direction rather than keeping the earlier "borrow then upgrade" approach:
This keeps the original crash fix intact while addressing the review concerns about ambiguous ownership transitions |
Description
Fixes LadybugDB/ladybug#290.
NodeQueryResultcurrently hands child results from a multi-statement chain to new wrappers as borrowed raw pointers. If the parent wrapper owns the chain and is closed first, the child wrapper can still call into freed native state and segfault.This patch changes the wrapper ownership model so detached child results keep their own native
QueryResultownership instead of borrowing the parent-owned chain node.It also adds a regression test for the sequence that previously crashed:
Types of changes
Checklist
Validation
I previously reproduced the crash in the Ladybug superproject at
v0.15.1and confirmed this fix removes the segfault in both:I did not rerun the full standalone
ladybug-nodejstest suite here because the local investigation environment and harness were set up in the Ladybug superproject worktree.