refactor(cli): use new sanitizer for resources #22125
Conversation
mmastrac
changed the title
| [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] |
There was a problem hiding this comment.
This PR changes the order of the output but I don't think we should care about that from a test perspective (allowing me to tweak in the future).
The information is what's important here, not the order IMO
| // TODO(mmastrac): until we implement the new timers and op sanitization, these must be ignored in this path | ||
| if item_name == "timer" { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
What's the consequence of it? Are timers ignored from sanitization altogether?
There was a problem hiding this comment.
Nope, they are picked up via op sanitization. Timers technically leak both an op and a resource and we were only showing the op before as a side effect of how things were implemented
| 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"), |
There was a problem hiding this comment.
Can you think of a way to make this data be defined on the Resource trait - that way we can force via type system to have these descriptions. I'm not sure how we'd look them up though.
There was a problem hiding this comment.
I think we could add a metadata block for resources but it would require a bit of work to add some registration macros.
| 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.", |
| let op_id_host_recv_message = ops | ||
| .iter() | ||
| .position(|op| *op == "op_host_recv_message") | ||
| .unwrap(); | ||
| let op_id_host_recv_ctrl = ops | ||
| .iter() | ||
| .position(|op| *op == "op_host_recv_ctrl") | ||
| .unwrap(); |
There was a problem hiding this comment.
Why are these special cased?
There was a problem hiding this comment.
These are a special case for the existing op sanitization as they are "weird" ops that may appear to leak. These will be used in the follow-up PR.
| // TODO(mmastrac): we should provide an API to poll the event loop until no futher | ||
| // progress is made. |
There was a problem hiding this comment.
This comment is a bit unclear to me - how would we figure out that "no further progress" is made?
There was a problem hiding this comment.
It would be something like an option to the poll method -- if the option is set we would return "ready" if the event loop did not make progress on ops, modules, timers or anything else (ie dispatch).
We currently return pending whether we did work or not and this would allow a caller to determine when the runtime has "settled" into a steady state.
This probably requires some research.
There was a problem hiding this comment.
Should I go ahead with the refactor we discussed in the past, where poll_event_loop would return an enum describing state of the event loop instead of Poll::Pending/Poll::Ready?
This reverts commit 84fb2ad.
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.
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.
We needed to revert the resource sanitizer in denoland/deno#22125 because of a panic in third-party test code. The problem was that in certain cases, if the OpDriver was kept alive either by Rc<ContextState> (in older code) or Rc<OpDriverImpl> (in newer code), the Tokio task that polls the underlying ops would potentially poll an op that expected something to be in OpState, but we had already removed all the items in the OpState by that point.
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.
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.
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.
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.
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)`
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)`
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.