Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deno.exit terminates entire process when using run --watch #7590

Open
sidd opened this issue Sep 20, 2020 · 21 comments
Open

Deno.exit terminates entire process when using run --watch #7590

sidd opened this issue Sep 20, 2020 · 21 comments
Assignees
Labels
--watch related to the watch feature of the CLI bug Something isn't working correctly cli related to cli/ dir help wanted community help requested

Comments

@sidd
Copy link
Contributor

sidd commented Sep 20, 2020

I'm running my script with deno run --watch --unstable myscript.ts. When it encounters a call to Deno.exit, the script terminates, including the file watcher.

I expected Deno.exit to terminate the script, but keep the file watcher alive. This is how Node's process.exit behaves when used with nodemon.

It seems culprit is the current implementation, which is just a passthrough to std::process::exit. I'm still familiarizing myself with Deno and V8, but is the solution something like tearing down the active runtime without exiting the main process?

@kitsonk kitsonk added the suggestion suggestions for new features (yet to be agreed) label Sep 20, 2020
@kitsonk
Copy link
Contributor

kitsonk commented Sep 20, 2020

Keep the file watcher alive to do what?

@sidd
Copy link
Contributor Author

sidd commented Sep 20, 2020

I thought Deno would continue to check for file changes, even after my script calls Deno.exit. Instead, I need to re-run deno run --watch --unstable myscript.ts manually. Minimal example of my script/output:

let someErrorCondition = true;

if (someErrorCondition) {
  console.log('Exiting');
  Deno.exit(1);
}

console.log('Finished');
$ deno --version
deno 1.4.1
v8 8.7.75
typescript 4.0.2
$ deno run --unstable --watch main.ts
Check file:///Users/sidd/code/issue/main.ts
Finished
Watcher Process terminated! Restarting on file change...
Watcher File change detected! Restarting!
Check file:///Users/sidd/code/issue/main.ts
Exiting
$

I expected to see the "Process terminated! Restarting on file change..." message after my script logged "Exiting", so I could make some changes to the script, and the script would re-run.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 20, 2020

If you exit your programme, you exit your program. I personally would find it surprising that if my programme has exited, that it suddenly restarts.

Like a "watch" in a browser, if the code changes and the page is still open, it reloads. If you close the page, you close the page. Anything else would be surprising in my opinion.

@sidd
Copy link
Contributor Author

sidd commented Sep 20, 2020

I see, I thought my use case was the most common. This is totally possible to solve in userland, so I can use some other tool to get quick feedback while developing.

I was surprised because tools in other environments continue to watch for files after their corresponding "exit" method is called, like cargo watch in Rust, nodemon in Node, and Guard in Ruby.

@lucacasonato
Copy link
Member

I agree that this is pretty unexpected - when the program exits by natural means (no more tasks to do), it also restarts when the file changes. I think that should also apply to Deno.exit(). The program should not be concerned with if it was started in --watch mode or not.

@lucacasonato lucacasonato added bug Something isn't working correctly cli related to cli/ dir and removed suggestion suggestions for new features (yet to be agreed) labels Sep 21, 2020
@bartlomieju bartlomieju mentioned this issue Oct 10, 2020
22 tasks
@magurotuna
Copy link
Member

May I work on this issue?

@bartlomieju
Copy link
Member

I agree with @lucacasonato and I think this is a reasonable feature to implement. If anyone wants to work on this feature I'll be happy to provide guidance on how to handle that, but long story short, we should override op_exit op if we are in --watch mode.

@bartlomieju bartlomieju added the help wanted community help requested label Dec 6, 2021
@bartlomieju bartlomieju added the --watch related to the watch feature of the CLI label Dec 11, 2021
@jespertheend
Copy link
Contributor

I'd like to work on this but I could use some pointers :)

@bartlomieju
Copy link
Member

@jespertheend I'm going to work on #12938 this week, which will require similar effort to this issue. I'll add pointers on how to tackle this issue after finishing my PR.

@bartlomieju
Copy link
Member

@jespertheend sorry for late response. I did some experiments in #12938 and although I have some idea how we could tackle this issue, it's definitely on the "very hard" spectrum of the problem; so I'm not sure I can provide pointers on how to implement that. I will be tackling #13093 in this quarter, after that issues is addressed fixing this issue will be much more approachable.

@jespertheend
Copy link
Contributor

That's alright, I think it's better to wait then. I only wanted to work on this to get a better understanding of Rust so it's not a priority for me.

@justinmchase
Copy link
Contributor

justinmchase commented Apr 9, 2022

It seems like in this case deno should create a child process with the exact same arguments sans --watch. The main process should then monitor the child process. If a file changes before the child process exits, then the parent should kill the child process. Once the child process is exited, deno should re-create the child process or wait for a file to change.

If the parent process receives any signals, it should forward them to the child process and once the child exits the main process should exit.

@justinmchase
Copy link
Contributor

justinmchase commented May 22, 2022

Is there a way to detect that deno is running in watch mode? It would be a helpful workaround, I could just not call deno.exit in that case.

@bartlomieju
Copy link
Member

Is there a way to detect that deno is running in watch mode for now? It would be a helpful workaround for now, I could just not call deno.exit in that case for now.

Unfortunately there is no way to tell that. I'll try to make time to fix this issue this week.

@bartlomieju
Copy link
Member

@justinmchase opened #14787

@kamilogorek
Copy link
Contributor

Is #14787 still the PR that will effectively resolve this issue? If it's reopened and merged of course :)

@bartlomieju
Copy link
Member

Is #14787 still the PR that will effectively resolve this issue? If it's reopened and merged of course :)

Possibly, but I haven't found a reliable way to handle Deno.exit() to terminate execution and do cleanup before calling a relevant syscall. I might revisit it in the future

@tsujp
Copy link

tsujp commented Aug 7, 2023

Relating to #14787 (comment) I recently had to whip up a quick test harness and part of that harness were some pre-checks which I wanted to abort on if they failed, so Deno.exit(1). Those pre-checks are not part of the test suite but rather the harness; (1) do checks (2) if all is well do tests.

Using a task and Deno.spawn() you can get something which auto-reruns when files change (e.g. as you red/green test your codebase) but which does not infinitely re-run upon a pre-check Deno.exit(1).


Here's my implementation in-situ but also summarised inline below:

./test/utility/runner.ts

// Calling Deno.exit() with any status code will prevent `Deno --watch` from
//   re-executing, so for the development task use a thin wrapper.

import { TextLineStream } from 'https://deno.land/[email protected]/streams/text_line_stream.ts'

const wd = Deno.cwd()

// * * * * * * * * * * * * * * * * * * * * * * * PRE-CHECK EXECUTION
// * * * * * * * * * * * * *
const precheck_cmd = new Deno.Command('deno', {
   cwd: wd,
   args: ['task', 'test:pre-check'],
   stdout: 'piped',
})

const precheck_proc = precheck_cmd.spawn()

// Decode stdout buffer stream.
const precheck_read = precheck_proc.stdout.pipeThrough(new TextDecoderStream())
   .pipeThrough(new TextLineStream())

// Print each line of stdout buffer stream; blocking lower (in file) execution.
for await (const l of precheck_read) {
   console.log(l)
}

// * * * * * * * * * * * * * * * * * * * * * * * TEST SUITE EXECUTION
// * * * * * * * * * * * * *

// If precheck process exits with success then the test suite can be started.
const precheck_success = await precheck_proc.status.then(({ success }) => success)

if (precheck_success) {
   const test_cmd = new Deno.Command('deno', {
      cwd: wd,
      args: ['task', 'test', '--fail-fast'],
      stdout: 'piped',
   })

   const test_proc = test_cmd.spawn()

   const test_read = test_proc
      .stdout
      .pipeThrough(new TextDecoderStream())
      .pipeThrough(new TextLineStream())

   for await (const l of test_read) {
      console.log(l)
   }

   await test_proc.status
}

./Deno.jsonc

{
    "imports": {
        // @deno/bdd and @deno/asserts
    },
    "tasks": {
        "dev": "deno run --allow-read --allow-run --watch=. ./test/utility/runner.ts",
        "test": "deno test --allow-read --allow-run",
        "test:pre-check": "deno run --allow-read --allow-run ./test/utility/precheck.ts",
    },
    "test": {
        "include": [
            "./test/*.ts",
        ],
        "exclude": [
            "./test/utility/*.ts"
        ]
    }
}

Summary

So in summary:

  • deno task dev watches every file (--watch=.)
  • Whenever there is a change ./test/utility/runner.ts is executed
  • runner.ts executes task deno task:pre-check (i.e. ./test/utility/precheck.ts)
  • precheck.ts may invoke Deno.exit(1) signalling failure...
    • If precheck.ts fails runner.ts does nothing else
    • If precheck.ts succeeded runner.ts executes task deno test.

@pschiffmann
Copy link

I just found this thread while dealing with a different issue. For me, the current behaviour of Deno.exit() is actually desired/useful.

TL;DR

Calling Deno.addSignalListener("SIGINT", ...) makes it so CTRL-C no longer terminates a deno run --watch process. Deno.exit() is a workaround to stop the watcher. If you change the --watch/Deno.exit() interaction, please provide an alternative way to terminate a --watch process with signal listeners.

Long version

Here is a real example, where I encountered this issue.

// server.ts
const kv = await Deno.openKv("./db.sqlite");

const server = Deno.serve(async () => {
  await kv.atomic().sum(["visitor_count"], 1n).commit();
  const { value } = await kv.get(["visitor_count"]);
  return new Response(`Hello world! You are the ${value}th visitor.`, { status: 200 });
});

I built an HTTP server with Deno.serve() and Deno KV. I run this server locally with deno run --unstable -A ./server.ts. When I'm done, I stop the server with CTRL-C. But stopping the server in this way leaves behind some KV temp files: db.sqlite-shm and db.sqlite-wal.
So I added the following code for a clean shutdown when I press CTRL-C:

// server.ts continuation
Deno.addSignalListener("SIGINT", async () => {
  kv.close();
  await server.shutdown();
});

This performs a clean KV shutdown when I press CTRL-C, and leaves no KV temp files behind on my hard drive.

However, once I run this with --watch, the process can't be stopped with CTRL-C at all anymore! Pressing CTRL-C will close the KV and stop the HTTP server, but then Deno prints this message to stdout: Watcher Process finished. Restarting on file change... and I need to kill the deno process. (I'm using deno 1.38.5 by the way.)
My workaround for now is to add a Deno.exit() call inside the signal listener.

@justinmchase
Copy link
Contributor

@pschiffmann That makes sense.

It seems almost like it would be beneficial for the signal listener to continue on with its normal behavior after the callback function is finished executing.

Your function is async, if the returned promise was awaited inside the Deno runtime and then continued on with handling SIGINT as normal then that would allow the user to override the behavior still by just not returning and also in your case what you want is for the normal Ctrl+C behavior to happen after you're done shutting down but without having to call exit.

Because it seems like the cruxt of the problem in this case is that when Ctrl+C signal is sent to the Deno process the intent is for everything to shutdown but when a Deno.exit() is called from within the watched application I would kind of expect it to kill the app child process but not the top level deno process which should continue watching for files.

@pschiffmann
Copy link

@justinmchase Full disclosure, I'm not super educated on OS processes, like what idiomatic or spec compliant signal handling behaviour looks like. I'm coming to deno from a web/JS background. So I don't have an opinion on how this API should work. I just encountered an edge case that I didn't see mentioned here, and thought I'd bring it up. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--watch related to the watch feature of the CLI bug Something isn't working correctly cli related to cli/ dir help wanted community help requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants