Skip to content
This repository was archived by the owner on Dec 24, 2025. It is now read-only.

Conversation

@mocchira
Copy link
Contributor

@mocchira mocchira commented Aug 5, 2025

No description provided.

@mocchira mocchira self-assigned this Aug 5, 2025
@mocchira mocchira requested a review from Copilot August 5, 2025 09:17
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a new approach for Deno.exit() functionality that provides controlled script termination instead of dangerous process exit. The change replaces the previous implementation that used std::process::exit() with a V8 isolate termination mechanism that stops JavaScript execution immediately while keeping the runtime alive.

  • Replaces dangerous std::process::exit() with controlled V8 isolate termination
  • Adds comprehensive exit request handling and runtime state management
  • Introduces extensive test coverage for various exit scenarios including infinite loops

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/op_whitelist.js Updates operation name from op_rustyscript_exit to op_script_exit
src/inner_runtime.rs Adds V8 isolate handle storage and script exit handling logic throughout runtime operations
src/ext/os/mod.rs Replaces process exit with V8 termination mechanism using OpState for exit requests
src/ext/os/init_os.js Updates JavaScript implementation to use new operation and adds unload event dispatch
src/ext/os/test.rs Adds comprehensive tests for script termination scenarios
src/error.rs Introduces new ScriptExit error variant with helper methods
examples/os_exit.rs Expands example with extensive testing of exit scenarios and zero-tolerance validation
Comments suppressed due to low confidence (2)

src/ext/os/test.rs:21

  • The parameter validation test has been weakened significantly. The new test only checks function.length instead of actually validating that invalid parameters are rejected, which reduces test coverage for error handling.
            // Test parameter validation by checking function signature

src/ext/os/test.rs:28

  • This test accepts both 0 and 1 parameters as valid, but doesn't verify the actual parameter validation logic in the implementation. Consider testing the actual validation behavior in a controlled way.
                param_validation = Deno.exit.length === 1 || Deno.exit.length === 0;

@mocchira mocchira force-pushed the ysh/add-script-exit branch from e7791b6 to 69c623d Compare August 5, 2025 10:06
@mocchira mocchira marked this pull request as ready for review August 5, 2025 10:15
@mocchira mocchira requested a review from a team August 5, 2025 10:15
}

println!("Success! The os_exit feature is working correctly.");
println!("JavaScript code now has access to Deno.exit() for script termination.");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ランタイム自体が Deno と互換性がある状態なので Node 互換の os.exit ではなく Deno.exit と命名しています

Copy link

@k1LoW k1LoW Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

質問です。Deno.exit という名前は露出はしないという認識ですがあっていますか?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ドキュメント上では露出しませんが、呼び出し自体は可能です。
スクリプトを終了させるだけなので、仮に呼び出し可能とわかってもセキュリティ的に問題はない認識です。

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Deno" という固有名詞が入っても問題ないかどうか気になったのでした。
仮にランタイムがNode.jsになった場合 "Node.exit" にする必要がでそうだと思ったのですが合っていますか?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deno_core に依存しているので && Node 互換の JS ランタイム ラッパーが rust に存在いないので、今後ランタイムが Node 互換になることはないです。

pub struct V8IsolateHandle(pub Rc<deno_core::v8::IsolateHandle>);

/// Request script termination with the given exit code (replaces dangerous std::process::exit)
/// This terminates V8 execution immediately for zero-tolerance termination
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここがメインです

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::process::exit(code); の利用から変更した。なるほど!

Copy link

@k1LoW k1LoW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

pub struct V8IsolateHandle(pub Rc<deno_core::v8::IsolateHandle>);

/// Request script termination with the given exit code (replaces dangerous std::process::exit)
/// This terminates V8 execution immediately for zero-tolerance termination
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::process::exit(code); の利用から変更した。なるほど!

Copy link

@remiposo remiposo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q]

@mocchira
Copy link
Contributor Author

mocchira commented Aug 6, 2025

@remiposo

Sorry for the basic question, but what was the reason we can't use deno_os's exit implementation?
https://github.com/denoland/deno/blob/5612d2edc7262d7bfb3bcfb0f18649b01ec94b53/ext/os/30_os.js#L77-L99

こちら最終的には std::process::exit(code); を呼んでいて OS プロセス自体を終了させてしまうため、今回の用途( Rust 側にコントロールを返す)には適していませんでした。
Rust 側にコントロールを返し且つ、exit が呼ばれたことを Rust に知らせるために今回の実装に変更する必要があります。

The reason parameter doesn't seem to be used - is it necessary?

これはデバッグ時の消し忘れですね!
消しておきます!

@mocchira
Copy link
Contributor Author

mocchira commented Aug 6, 2025

お二方レビューありがとうございました!
マージします

@mocchira mocchira merged commit e763277 into master Aug 6, 2025
5 checks passed
@mocchira mocchira deleted the ysh/add-script-exit branch August 6, 2025 09:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants