From e20113a0eb8622e27f2ed203f2b628c3d2f7e5f5 Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Wed, 15 May 2024 22:59:10 +0000 Subject: [PATCH] Remove stack debug assert (#12861) # Description In order for `Stack::unwrap_unique` to work as intended, we currently manually track all references to the parent stack and ensure that they are cleared before calling `Stack::unwrap_unique` in the REPL. We also only call `Stack::unwrap_unique` after all code from the current REPL entry has finished executing. Since `Value`s cannot store `Stack` references, then this should have worked in theory. However, we forgot to account for threads. `run-external` (and maybe the plugin writers) can spawn threads that clone the `Stack`, holding on to references of the parent stack. These threads are not waited/joined upon, and so may finish after the eval has already returned. This PR removes the `Stack::unwrap_unique` function and associated debug assert that was [causing panics](https://gist.github.com/cablehead/f3d2608a1629e607c2d75290829354f7) like @cablehead found. # After Submitting Make values cheaper to clone as a more robust solution to the performance issues with cloning the stack. --------- Co-authored-by: Wind --- crates/nu-cli/src/repl.rs | 4 +++- crates/nu-protocol/src/engine/stack.rs | 32 ++++++-------------------- 2 files changed, 10 insertions(+), 26 deletions(-) diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index 2482a7920e..29c2f62734 100644 --- a/crates/nu-cli/src/repl.rs +++ b/crates/nu-cli/src/repl.rs @@ -542,7 +542,9 @@ fn loop_iteration(ctx: LoopContext) -> (bool, Stack, Reedline) { let shell_integration_osc633 = config.shell_integration_osc633; let shell_integration_reset_application_mode = config.shell_integration_reset_application_mode; - let mut stack = Stack::unwrap_unique(stack_arc); + // TODO: we may clone the stack, this can lead to major performance issues + // so we should avoid it or making stack cheaper to clone. + let mut stack = Arc::unwrap_or_clone(stack_arc); perf( "line_editor setup", diff --git a/crates/nu-protocol/src/engine/stack.rs b/crates/nu-protocol/src/engine/stack.rs index 65f2981fbf..19726db9c0 100644 --- a/crates/nu-protocol/src/engine/stack.rs +++ b/crates/nu-protocol/src/engine/stack.rs @@ -75,26 +75,10 @@ impl Stack { } } - /// Unwrap a uniquely-owned stack. - /// - /// In debug mode, this panics if there are multiple references. - /// In production this will instead clone the underlying stack. - pub fn unwrap_unique(stack_arc: Arc) -> Stack { - // If you hit an error here, it's likely that you created an extra - // Arc pointing to the stack somewhere. Make sure that it gets dropped before - // getting here! - Arc::try_unwrap(stack_arc).unwrap_or_else(|arc| { - // in release mode, we clone the stack, but this can lead to - // major performance issues, so we should avoid it - debug_assert!(false, "More than one stack reference remaining!"); - (*arc).clone() - }) - } - /// Create a new child stack from a parent. /// /// Changes from this child can be merged back into the parent with - /// Stack::with_changes_from_child + /// [`Stack::with_changes_from_child`] pub fn with_parent(parent: Arc) -> Stack { Stack { // here we are still cloning environment variable-related information @@ -109,19 +93,17 @@ impl Stack { } } - /// Take an Arc of a parent (assumed to be unique), and a child, and apply - /// all the changes from a child back to the parent. + /// Take an [`Arc`] parent, and a child, and apply all the changes from a child back to the parent. /// - /// Here it is assumed that child was created with a call to Stack::with_parent - /// with parent + /// Here it is assumed that `child` was created by a call to [`Stack::with_parent`] with `parent`. + /// + /// For this to be performant and not clone `parent`, `child` should be the only other + /// referencer of `parent`. pub fn with_changes_from_child(parent: Arc, child: Stack) -> Stack { // we're going to drop the link to the parent stack on our new stack // so that we can unwrap the Arc as a unique reference - // - // This makes the new_stack be in a bit of a weird state, so we shouldn't call - // any structs drop(child.parent_stack); - let mut unique_stack = Stack::unwrap_unique(parent); + let mut unique_stack = Arc::unwrap_or_clone(parent); unique_stack .vars