fix: op_set_exit_code #13034
fix: op_set_exit_code #13034
Conversation
|
@bnoordhuis please take a look |
|
|
||
| let code = deno_runtime::EXIT_CODE.load(Relaxed); | ||
| std::process::exit(code); | ||
| std::process::exit(exit_code); |
There was a problem hiding this comment.
I think we should only call exit if exit_code is not equal to 0. Otherwise drop handlers will not be called which might lead to zombie subprocesses.
There was a problem hiding this comment.
What drop handlers are you worried about? The real work is scoped to run_basic(), right?
There was a problem hiding this comment.
All drop handlers - IIRC if you call std::process:exit the process immediately terminates and no drop handlers are run; when we run a subprocess using Deno.run() we got close_on_drop set to true, and when you are existing using exit() these drop handlers will not run and subprocesses will not close becoming zombies.
There was a problem hiding this comment.
I'm still confused. Are you talking about lexically scoped drop handlers? main() doesn't have any that matter, does it?
(I'm also confused by your close_on_drop comment - where is that a thing inside the source tree?)
There was a problem hiding this comment.
Please scratch that, I should have commented before the first coffee :) of course drop handlers run because they are scoped to run_basic(). And in case of close_on_drop I actually meant kill_on_drop
| // TODO(bartlomieju): this function is not handling `exit_code` set by the runtime | ||
| // code properly. | ||
| async fn run_with_watch(flags: Flags, script: String) -> Result<i32, AnyError> { |
There was a problem hiding this comment.
Should be addressed in tandem with #7590
|
|
||
| let code = deno_runtime::EXIT_CODE.load(Relaxed); | ||
| std::process::exit(code); | ||
| std::process::exit(exit_code); |
There was a problem hiding this comment.
What drop handlers are you worried about? The real work is scoped to run_basic(), right?
|
|
||
| pub fn init() -> Extension { | ||
| pub fn init(maybe_exit_code: Option<Arc<AtomicI32>>) -> Extension { |
There was a problem hiding this comment.
Does it have to be an Arc? It's only accessed from the same thread, I think.
There was a problem hiding this comment.
It has to, it's shares between all created threads (ie. web workers).
|
I'm going to land this PR and we can address other questions at a later point. |
Fixes "op_set_exit_code" by sharing a single "Arc" between
all workers (via "op state") instead of having a "global" value stored in
"deno_runtime" crate. As a consequence setting an exit code is always
scoped to a tree of workers, instead of being overridable if there are
multiple worker tree (like in "deno test --jobs" subcommand).
Refactored "cli/main.rs" functions to return "Result<i32, AnyError>" instead
of "Result<(), AnyError>" so they can return exit code.
Ref #12938