Reland refactor(cli): use new sanitizer for resources #22226

mmastrac · 2024-02-01T19:04:41Z

Originally in #22125
Reverted in #22153 because of #22148

Fixed in deno_core denoland/deno_core#538

Test plan:

Check out: https://github.com/poolifier/poolifier-deno.git
PATH=.../deno/target/release/:$PATH deno task test
ok | 13 passed (188 steps) | 0 failed (18s)

bartlomieju · 2024-02-05T01:17:33Z

cli/tools/test/fmt.rs

+fn pretty_resource_name(
+  name: &str,
+) -> (Cow<'static, str>, &'static str, &'static str) {
+  let (name, action1, action2) = match name {
+    "fsFile" => ("A file", "opened", "closed"),
+    "fetchRequest" => ("A fetch request", "started", "finished"),
+    "fetchRequestBody" => ("A fetch request body", "created", "closed"),
+    "fetchResponse" => ("A fetch response body", "created", "consumed"),
+    "httpClient" => ("An HTTP client", "created", "closed"),
+    "dynamicLibrary" => ("A dynamic library", "loaded", "unloaded"),
+    "httpConn" => ("An inbound HTTP connection", "accepted", "closed"),
+    "httpStream" => ("An inbound HTTP request", "accepted", "closed"),
+    "tcpStream" => ("A TCP connection", "opened/accepted", "closed"),
+    "unixStream" => ("A Unix connection", "opened/accepted", "closed"),
+    "tlsStream" => ("A TLS connection", "opened/accepted", "closed"),
+    "tlsListener" => ("A TLS listener", "opened", "closed"),
+    "unixListener" => ("A Unix listener", "opened", "closed"),
+    "unixDatagram" => ("A Unix datagram", "opened", "closed"),
+    "tcpListener" => ("A TCP listener", "opened", "closed"),
+    "udpSocket" => ("A UDP socket", "opened", "closed"),
+    "timer" => ("A timer", "started", "fired/cleared"),
+    "textDecoder" => ("A text decoder", "created", "finished"),
+    "messagePort" => ("A message port", "created", "closed"),
+    "webSocketStream" => ("A WebSocket", "opened", "closed"),
+    "fsEvents" => ("A file system watcher", "created", "closed"),
+    "childStdin" => ("A child process stdin", "opened", "closed"),
+    "childStdout" => ("A child process stdout", "opened", "closed"),
+    "childStderr" => ("A child process stderr", "opened", "closed"),
+    "child" => ("A child process", "started", "closed"),
+    "signal" => ("A signal listener", "created", "fired/cleared"),
+    "stdin" => ("The stdin pipe", "opened", "closed"),
+    "stdout" => ("The stdout pipe", "opened", "closed"),
+    "stderr" => ("The stderr pipe", "opened", "closed"),
+    "compression" => ("A CompressionStream", "created", "closed"),
+    _ => return (format!("\"{name}\"").into(), "created", "cleaned up"),
+  };
+  (name.into(), action1, action2)
+}
+
+fn resource_close_hint(name: &str) -> &'static str {
+  match name {
+    "fsFile" => "Close the file handle by calling `file.close()`.",
+    "fetchRequest" => "Await the promise returned from `fetch()` or abort the fetch with an abort signal.",
+    "fetchRequestBody" => "Terminate the request body `ReadableStream` by closing or erroring it.",
+    "fetchResponse" => "Consume or close the response body `ReadableStream`, e.g `await resp.text()` or `await resp.body.cancel()`.",
+    "httpClient" => "Close the HTTP client by calling `httpClient.close()`.",
+    "dynamicLibrary" => "Unload the dynamic library by calling `dynamicLibrary.close()`.",
+    "httpConn" => "Close the inbound HTTP connection by calling `httpConn.close()`.",
+    "httpStream" => "Close the inbound HTTP request by responding with `e.respondWith()` or closing the HTTP connection.",
+    "tcpStream" => "Close the TCP connection by calling `tcpConn.close()`.",
+    "unixStream" => "Close the Unix socket connection by calling `unixConn.close()`.",
+    "tlsStream" => "Close the TLS connection by calling `tlsConn.close()`.",
+    "tlsListener" => "Close the TLS listener by calling `tlsListener.close()`.",
+    "unixListener" => "Close the Unix socket listener by calling `unixListener.close()`.",
+    "unixDatagram" => "Close the Unix datagram socket by calling `unixDatagram.close()`.",
+    "tcpListener" => "Close the TCP listener by calling `tcpListener.close()`.",
+    "udpSocket" => "Close the UDP socket by calling `udpSocket.close()`.",
+    "timer" => "Clear the timer by calling `clearInterval` or `clearTimeout`.",
+    "textDecoder" => "Close the text decoder by calling `textDecoder.decode('')` or `await textDecoderStream.readable.cancel()`.",
+    "messagePort" => "Close the message port by calling `messagePort.close()`.",
+    "webSocketStream" => "Close the WebSocket by calling `webSocket.close()`.",
+    "fsEvents" => "Close the file system watcher by calling `watcher.close()`.",
+    "childStdin" => "Close the child process stdin by calling `proc.stdin.close()`.",
+    "childStdout" => "Close the child process stdout by calling `proc.stdout.close()` or `await child.stdout.cancel()`.",
+    "childStderr" => "Close the child process stderr by calling `proc.stderr.close()` or `await child.stderr.cancel()`.",
+    "child" => "Close the child process by calling `proc.kill()` or `proc.close()`.",
+    "signal" => "Clear the signal listener by calling `Deno.removeSignalListener`.",
+    "stdin" => "Close the stdin pipe by calling `Deno.stdin.close()`.",
+    "stdout" => "Close the stdout pipe by calling `Deno.stdout.close()`.",
+    "stderr" => "Close the stderr pipe by calling `Deno.stderr.close()`.",
+    "compression" => "Close the compression stream by calling `await stream.writable.close()`.",
+    _ => "Close the resource before the end of the test.",
+  }
+}


Consider doing these in a separate PR - in case we need to revert again, we'll keep diffs small and we know this code is ok

Might even add relevant ops to reach for this data into Rust, so we can apply most of the changes from 40_test.js?

cli/tools/test/mod.rs

cli/tools/test/reporters/common.rs

Extract zero-risk changes from #22226

Step 1 of the Rustification of sanitizers, which unblocks the faster timers. This replaces the resource sanitizer with a Rust one, using the new APIs in deno_core.

Extract zero-risk changes from #22226

Originally in #22125 Reverted in #22153 because of #22148 Fixed in deno_core denoland/deno_core#538 Test plan: 1. Check out: https://github.com/poolifier/poolifier-deno.git 2. `PATH=.../deno/target/release/:$PATH deno task test` 3. `ok | 13 passed (188 steps) | 0 failed (18s)`

The format of the sanitizers will change a little bit: - If multiple async ops leak and traces are on, we repeat the async op header once per stack trace. - All leaks are aggregated under a "Leaks detected:" banner as the new timers are eventually going to be added, and these are neither ops nor resources. - `1 async op` is now `An async op` - If ops and resources leak, we show both (rather than op leaks masking resources) Follow-on to #22226

mmastrac changed the title ~~Re-land refactor(cli): use new sanitizer for resources [WIP]~~ Reland refactor(cli): use new sanitizer for resources [WIP] Feb 1, 2024

mmastrac force-pushed the resource_sanitizer_2 branch from eb69100 to a99a452 Compare February 3, 2024 00:01

bartlomieju reviewed Feb 5, 2024

View reviewed changes

mmastrac mentioned this pull request Feb 5, 2024

refactor(cli): Add TestFailureDescription #22267

Merged

mmastrac force-pushed the resource_sanitizer_2 branch 2 times, most recently from c20a3d1 to f22bd06 Compare February 5, 2024 17:09

mmastrac added a commit that referenced this pull request Feb 5, 2024

refactor(cli): Add TestFailureDescription (#22267)

aecad7f

Extract zero-risk changes from #22226

refactor(cli): use new sanitizer for resources (denoland#22125)

6bf85fa

Step 1 of the Rustification of sanitizers, which unblocks the faster timers. This replaces the resource sanitizer with a Rust one, using the new APIs in deno_core.

mmastrac force-pushed the resource_sanitizer_2 branch from f22bd06 to 6bf85fa Compare February 5, 2024 17:34

mmastrac added 2 commits February 5, 2024 10:50

feedback

5928b85

.

33d037d

mmastrac changed the title ~~Reland refactor(cli): use new sanitizer for resources [WIP]~~ Reland refactor(cli): use new sanitizer for resources Feb 5, 2024

remove op sanitizer stuff

a3e6108

mmastrac requested a review from bartlomieju February 5, 2024 18:39

bartlomieju approved these changes Feb 5, 2024

View reviewed changes

mmastrac merged commit 0a3d329 into denoland:main Feb 5, 2024
15 checks passed

littledivy pushed a commit that referenced this pull request Feb 8, 2024

refactor(cli): Add TestFailureDescription (#22267)

fbe8f0d

Extract zero-risk changes from #22226

mmastrac mentioned this pull request Feb 14, 2024

refactor(cli): move op sanitizer to Rust #22413

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland refactor(cli): use new sanitizer for resources #22226

Reland refactor(cli): use new sanitizer for resources #22226

mmastrac commented Feb 1, 2024 •

edited

bartlomieju Feb 5, 2024

bartlomieju Feb 5, 2024

Reland refactor(cli): use new sanitizer for resources #22226

Reland refactor(cli): use new sanitizer for resources #22226

Conversation

mmastrac commented Feb 1, 2024 • edited

bartlomieju Feb 5, 2024

Choose a reason for hiding this comment

bartlomieju Feb 5, 2024

Choose a reason for hiding this comment

mmastrac commented Feb 1, 2024 •

edited