mirror of
https://github.com/nushell/nushell.git
synced 2024-11-25 09:53:43 +01:00
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 <WindSoilder@outlook.com>
This commit is contained in:
parent
6f3dbc97bb
commit
e20113a0eb
@ -542,7 +542,9 @@ fn loop_iteration(ctx: LoopContext) -> (bool, Stack, Reedline) {
|
|||||||
let shell_integration_osc633 = config.shell_integration_osc633;
|
let shell_integration_osc633 = config.shell_integration_osc633;
|
||||||
let shell_integration_reset_application_mode = config.shell_integration_reset_application_mode;
|
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(
|
perf(
|
||||||
"line_editor setup",
|
"line_editor setup",
|
||||||
|
@ -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>) -> 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.
|
/// Create a new child stack from a parent.
|
||||||
///
|
///
|
||||||
/// Changes from this child can be merged back into the parent with
|
/// 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 {
|
pub fn with_parent(parent: Arc<Stack>) -> Stack {
|
||||||
Stack {
|
Stack {
|
||||||
// here we are still cloning environment variable-related information
|
// 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
|
/// Take an [`Arc`] parent, and a child, and apply all the changes from a child back to the parent.
|
||||||
/// 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
|
/// Here it is assumed that `child` was created by a call to [`Stack::with_parent`] 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<Stack>, child: Stack) -> Stack {
|
pub fn with_changes_from_child(parent: Arc<Stack>, child: Stack) -> Stack {
|
||||||
// we're going to drop the link to the parent stack on our new 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
|
// 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);
|
drop(child.parent_stack);
|
||||||
let mut unique_stack = Stack::unwrap_unique(parent);
|
let mut unique_stack = Arc::unwrap_or_clone(parent);
|
||||||
|
|
||||||
unique_stack
|
unique_stack
|
||||||
.vars
|
.vars
|
||||||
|
Loading…
Reference in New Issue
Block a user