Skip to content

Commit

Permalink
refactor(cli): use new sanitizer for resources (#22125)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mmastrac authored and littledivy committed Feb 1, 2024
1 parent 382b98a commit 026fd49
Show file tree
Hide file tree
Showing 9 changed files with 239 additions and 204 deletions.
183 changes: 2 additions & 181 deletions cli/js/40_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,8 @@ const {
MapPrototypeGet,
MapPrototypeHas,
MapPrototypeSet,
ObjectKeys,
Promise,
SafeArrayIterator,
Set,
SymbolToStringTag,
TypeError,
} = primordials;
Expand Down Expand Up @@ -342,182 +340,6 @@ function assertOps(fn) {
};
}

function prettyResourceNames(name) {
switch (name) {
case "fsFile":
return ["A file", "opened", "closed"];
case "fetchRequest":
return ["A fetch request", "started", "finished"];
case "fetchRequestBody":
return ["A fetch request body", "created", "closed"];
case "fetchResponse":
return ["A fetch response body", "created", "consumed"];
case "httpClient":
return ["An HTTP client", "created", "closed"];
case "dynamicLibrary":
return ["A dynamic library", "loaded", "unloaded"];
case "httpConn":
return ["An inbound HTTP connection", "accepted", "closed"];
case "httpStream":
return ["An inbound HTTP request", "accepted", "closed"];
case "tcpStream":
return ["A TCP connection", "opened/accepted", "closed"];
case "unixStream":
return ["A Unix connection", "opened/accepted", "closed"];
case "tlsStream":
return ["A TLS connection", "opened/accepted", "closed"];
case "tlsListener":
return ["A TLS listener", "opened", "closed"];
case "unixListener":
return ["A Unix listener", "opened", "closed"];
case "unixDatagram":
return ["A Unix datagram", "opened", "closed"];
case "tcpListener":
return ["A TCP listener", "opened", "closed"];
case "udpSocket":
return ["A UDP socket", "opened", "closed"];
case "timer":
return ["A timer", "started", "fired/cleared"];
case "textDecoder":
return ["A text decoder", "created", "finished"];
case "messagePort":
return ["A message port", "created", "closed"];
case "webSocketStream":
return ["A WebSocket", "opened", "closed"];
case "fsEvents":
return ["A file system watcher", "created", "closed"];
case "childStdin":
return ["A child process stdin", "opened", "closed"];
case "childStdout":
return ["A child process stdout", "opened", "closed"];
case "childStderr":
return ["A child process stderr", "opened", "closed"];
case "child":
return ["A child process", "started", "closed"];
case "signal":
return ["A signal listener", "created", "fired/cleared"];
case "stdin":
return ["The stdin pipe", "opened", "closed"];
case "stdout":
return ["The stdout pipe", "opened", "closed"];
case "stderr":
return ["The stderr pipe", "opened", "closed"];
case "compression":
return ["A CompressionStream", "created", "closed"];
default:
return [`A "${name}" resource`, "created", "cleaned up"];
}
}

function resourceCloseHint(name) {
switch (name) {
case "fsFile":
return "Close the file handle by calling `file.close()`.";
case "fetchRequest":
return "Await the promise returned from `fetch()` or abort the fetch with an abort signal.";
case "fetchRequestBody":
return "Terminate the request body `ReadableStream` by closing or erroring it.";
case "fetchResponse":
return "Consume or close the response body `ReadableStream`, e.g `await resp.text()` or `await resp.body.cancel()`.";
case "httpClient":
return "Close the HTTP client by calling `httpClient.close()`.";
case "dynamicLibrary":
return "Unload the dynamic library by calling `dynamicLibrary.close()`.";
case "httpConn":
return "Close the inbound HTTP connection by calling `httpConn.close()`.";
case "httpStream":
return "Close the inbound HTTP request by responding with `e.respondWith()` or closing the HTTP connection.";
case "tcpStream":
return "Close the TCP connection by calling `tcpConn.close()`.";
case "unixStream":
return "Close the Unix socket connection by calling `unixConn.close()`.";
case "tlsStream":
return "Close the TLS connection by calling `tlsConn.close()`.";
case "tlsListener":
return "Close the TLS listener by calling `tlsListener.close()`.";
case "unixListener":
return "Close the Unix socket listener by calling `unixListener.close()`.";
case "unixDatagram":
return "Close the Unix datagram socket by calling `unixDatagram.close()`.";
case "tcpListener":
return "Close the TCP listener by calling `tcpListener.close()`.";
case "udpSocket":
return "Close the UDP socket by calling `udpSocket.close()`.";
case "timer":
return "Clear the timer by calling `clearInterval` or `clearTimeout`.";
case "textDecoder":
return "Close the text decoder by calling `textDecoder.decode('')` or `await textDecoderStream.readable.cancel()`.";
case "messagePort":
return "Close the message port by calling `messagePort.close()`.";
case "webSocketStream":
return "Close the WebSocket by calling `webSocket.close()`.";
case "fsEvents":
return "Close the file system watcher by calling `watcher.close()`.";
case "childStdin":
return "Close the child process stdin by calling `proc.stdin.close()`.";
case "childStdout":
return "Close the child process stdout by calling `proc.stdout.close()` or `await child.stdout.cancel()`.";
case "childStderr":
return "Close the child process stderr by calling `proc.stderr.close()` or `await child.stderr.cancel()`.";
case "child":
return "Close the child process by calling `proc.kill()` or `proc.close()`.";
case "signal":
return "Clear the signal listener by calling `Deno.removeSignalListener`.";
case "stdin":
return "Close the stdin pipe by calling `Deno.stdin.close()`.";
case "stdout":
return "Close the stdout pipe by calling `Deno.stdout.close()`.";
case "stderr":
return "Close the stderr pipe by calling `Deno.stderr.close()`.";
case "compression":
return "Close the compression stream by calling `await stream.writable.close()`.";
default:
return "Close the resource before the end of the test.";
}
}

// Wrap test function in additional assertion that makes sure
// the test case does not "leak" resources - ie. resource table after
// the test has exactly the same contents as before the test.
function assertResources(fn) {
/** @param desc {TestDescription | TestStepDescription} */
return async function resourceSanitizer(desc) {
const pre = core.resources();
const innerResult = await fn(desc);
if (innerResult) return innerResult;
const post = core.resources();

const allResources = new Set([
...new SafeArrayIterator(ObjectKeys(pre)),
...new SafeArrayIterator(ObjectKeys(post)),
]);

const details = [];
for (const resource of allResources) {
const preResource = pre[resource];
const postResource = post[resource];
if (preResource === postResource) continue;

if (preResource === undefined) {
const [name, action1, action2] = prettyResourceNames(postResource);
const hint = resourceCloseHint(postResource);
const detail =
`${name} (rid ${resource}) was ${action1} during the test, but not ${action2} during the test. ${hint}`;
ArrayPrototypePush(details, detail);
} else {
const [name, action1, action2] = prettyResourceNames(preResource);
const detail =
`${name} (rid ${resource}) was ${action1} before the test started, but was ${action2} during the test. Do not close resources in a test that were not created during that test.`;
ArrayPrototypePush(details, detail);
}
}
if (details.length == 0) {
return null;
}
return { failed: { leakedResources: details } };
};
}

// Wrap test function in additional assertion that makes sure
// that the test case does not accidentally exit prematurely.
function assertExit(fn, isTest) {
Expand Down Expand Up @@ -720,6 +542,8 @@ function testInner(
testDesc.name,
testDesc.ignore,
testDesc.only,
false, /*testDesc.sanitizeOps*/
testDesc.sanitizeResources,
testDesc.location.fileName,
testDesc.location.lineNumber,
testDesc.location.columnNumber,
Expand Down Expand Up @@ -910,9 +734,6 @@ function wrapTest(desc) {
if (desc.sanitizeOps) {
testFn = assertOps(testFn);
}
if (desc.sanitizeResources) {
testFn = assertResources(testFn);
}
if (desc.sanitizeExit) {
testFn = assertExit(testFn, true);
}
Expand Down
4 changes: 3 additions & 1 deletion cli/lsp/testing/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,9 @@ impl TestRun {
test::TestResult::Ignored => summary.ignored += 1,
test::TestResult::Failed(error) => {
summary.failed += 1;
summary.failures.push((description.clone(), error.clone()));
summary
.failures
.push(((&description).into(), error.clone()));
}
test::TestResult::Cancelled => {
summary.failed += 1;
Expand Down
4 changes: 4 additions & 0 deletions cli/ops/testing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ fn op_register_test(
#[string] name: String,
ignore: bool,
only: bool,
sanitize_ops: bool,
sanitize_resources: bool,
#[string] file_name: String,
#[smi] line_number: u32,
#[smi] column_number: u32,
Expand All @@ -141,6 +143,8 @@ fn op_register_test(
name,
ignore,
only,
sanitize_ops,
sanitize_resources,
origin: origin.clone(),
location: TestLocation {
file_name,
Expand Down
6 changes: 4 additions & 2 deletions cli/tests/testdata/test/resource_sanitizer.out
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ leak ... FAILED ([WILDCARD])

leak => ./test/resource_sanitizer.ts:[WILDCARD]
error: Leaking resources:
- The stdin pipe (rid 0) was opened before the test started, but was closed during the test. Do not close resources in a test that were not created during that test.
- A file (rid 3) was opened during the test, but not closed during the test. Close the file handle by calling `file.close()`.
[UNORDERED_START]
- The stdin pipe was opened before the test started, but was closed during the test. Do not close resources in a test that were not created during that test.
- A file was opened during the test, but not closed during the test. Close the file handle by calling `file.close()`.
[UNORDERED_END]

FAILURES

Expand Down

0 comments on commit 026fd49

Please sign in to comment.