feat: deno run --watch doesn't exit on Deno.exit()

[bartlomieju]

This PR changes behavior of Deno.exit() when --watch flag is passed. Instead of exiting the watcher process, it terminates JS execution but keeps file watcher alive.

This is done, by terminating JS execution from an "op_exit" instead of calling "std::process::exit()". In such case the event loop terminates immediately, but the flow of the Rust program continues - exit code is grabbed from worker's state and handled by the caller. For "deno run" subcommand it is used in "std::process::exit()" call, but for "deno run --watch" it is handled by file watcher to display what was the exit code and keep watching for file changes. This greatly improves DX when quickly iterating on code.

As a consequence "Drop" handlers are properly run which should help alleviate several reported issues. This will be done in follow up commits.

Closes https://github.com/denoland/deno/issues/7590

[lucacasonato]

Ideally we'd print a stack trace originating at the call site of Deno.exit()

I think we should print no stack trace, rather just show the exit code in the watcher message.

[justinmchase]

A couple questions:

1. Suppose I have some native modules, perhaps with threading, will they be stopped and freed appropriately when the core is shutdown too?

2. Can I get the isFileWatcher variable as code? Such as Deno.isFileWatcher ?

3. Will the process itself still respond to signals such as SIGINT correctly and exit as normal? This will not affect that code?

4. Can I also intercept the exit function? It would be useful in tests to be able to call exit without it stopping all the tests.

[justinmchase]

Related to @lucacasonato 's comment and the below output, here is a recommendation...

original

This program will exit with code 120
error: Uncaught Error: Process exited with code: 120
    at deno:runtime/js/99_main.js:591:24
Watcher Process finished. Restarting on file change...
Watcher File change detected! Restarting!

suggested

This program will exit with code 120
Application Process exited with code: 120. Restarting on file change...
Watcher File change detected! Restarting!

[bartlomieju]

A couple questions:

1. Suppose I have some native modules, perhaps with threading, will they be stopped and freed appropriately when the core is shutdown too?

No, that would require https://github.com/denoland/deno/issues/13093

2. Can I get the isFileWatcher variable as code? Such as Deno.isFileWatcher ?

I asked some team members about it and there's no support for adding such API at the moment. What's your use case for the API? Mine was to automatically enable client side HMR if we're running in watch mode.

3. Will the process itself still respond to signals such as SIGINT correctly and exit as normal? This will not affect that code?

The process will, but since the V8 isolate is terminated your JS code for handling these things will not work anymore.

4. Can I also intercept the exit function? It would be useful in tests to be able to call exit without it stopping all the tests.

You'd have to manually overwrite Deno.exit function. Actually deno test will not allow you to call Deno.exit() - it will instead throw an error from "exit sanitizer".

[justinmchase]

What's your use case for the API?

I'm just writing tests for some CLI code, and in some places Deno.exit(1) is called, so just trying to test that functionality with Deno.test.

I thought if I could detect that it was running in watch mode then I could simply not call exit and have it do something else instead. I could of course abstract that call and inject the exit functionality but it would likely be easier to just not actually call exit.

I'm hoping that Deno.exit(1) isn't actually just throwing an Error now? That may affect my tests... It looked like the error throwing was happening internally somewhere and being handled by the runtime, this will not manifest as a throw in my test code will it?

[bartlomieju]

I'm hoping that Deno.exit(1) isn't actually just throwing an Error now? That may affect my tests... It looked like the error throwing was happening internally somewhere and being handled by the runtime, this will not manifest as a throw in my test code will it?

It is, if you try to Deno.exit() inside a Deno.test() you will get an error like this:

exit(0) => ./test/exit_sanitizer.ts:[WILDCARD]
error: AssertionError: Test case attempted to exit with exit code: 0
  Deno.exit(0);
       ^
    at ....

[justinmchase]

Hmm, that's a little confusing but I guess so long as its deterministic, so long as that error is one that I can catch and detect in the test then I can add an expectation that it throws an ExitError or whatever its called. How would it look to write such a test?

I'm imagining

import { assert } from '...'
import { cli } from './main.ts'

Deno.test({
  fn: () => assert.throws(cli(['bad', 'args']), new ExitError(1))
})

[bartlomieju]

Hmm, that's a little confusing but I guess so long as its deterministic, so long as that error is one that I can catch and detect in the test then I can add an expectation that it throws an ExitError or whatever its called. How would it look to write such a test?

No, it doesn't throw an error that you can catch - if you call Deno.exit() inside a test it will be caught by exit sanitizer (https://deno.land/manual/testing/sanitizers#exit-sanitizer) - it's an internal helper in deno test that prevents accidental calls to Deno.exit() that might lead to false positives (imagine a test case calls Deno.exit(0) somewhere, even though you had a failure in previous test case, on CI that would case the pipeline to pass when it shouldn't).

Since you are writing a CLI I suggest to spawn subprocesses using (Deno.run or Deno.spawn API), in the test case to check for proper behavior and exit codes. If you need to check for output you might use snapshots to verify that output matches your expectations.

[stale[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.