refactor(cli): refactor bench/test for future module changes #21460
Conversation
| @@ -1,5 +1,9 @@ | |||
| error: Uncaught TypeError: Cannot read properties of undefined (reading 'fn') | |||
| error: TypeError: Cannot read properties of undefined (reading 'fn') | |||
There was a problem hiding this comment.
These messages may change as we continue refactoring -- it's a bit annoying that they change because of these refactorings but I'll try to stabilize them with the future work.
| Deno.bench(); | ||
| ^ | ||
| at [WILDCARD] | ||
| at [WILDCARD]/bench/no_check.ts:1:6 | ||
| This error was not caught from a benchmark and caused the bench runner to fail on the referenced module. |
There was a problem hiding this comment.
Makes the benchmark and test runner more consistent
mmastrac
force-pushed
the
bench_test_prefactor
branch
from
9c7f511
to
c1fe711
Compare
runtime/worker.rs
Outdated
| receiver.await | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Run the event loop up to a given duration. |
There was a problem hiding this comment.
I might promote this to deno_core but I would like to let it bake in deno for a bit
| Deno.bench(); | ||
| ^ | ||
| at [WILDCARD] | ||
| at [WILDCARD]/bench/no_check.ts:1:6 | ||
| This error was not caught from a benchmark and caused the bench runner to fail on the referenced module. |
| @@ -180,6 +214,10 @@ async fn bench_specifier( | |||
| worker.execute_side_module_possibly_with_npm().await?; | |||
|
|
|||
| let mut worker = worker.into_main_worker(); | |||
|
|
|||
| // Ensure that there are no pending exceptions before we start running tests | |||
| worker.run_up_to_duration(Duration::from_millis(0)).await?; | |||
There was a problem hiding this comment.
Isn't this racy? Ie. won't in some situations timeouts fire before event loop tick finishes?
There was a problem hiding this comment.
This turns out to catch 100% of the existing promise resolution failures we have today, so I left it at the minimum value that works. In the current system promise rejections are loaded only if they are raised in the same tick as a module resolution which is extremely brittle.
| // Ensure the worker has settled so we can catch any remaining unhandled rejections. We don't | ||
| // want to wait forever here. | ||
| worker.run_up_to_duration(Duration::from_millis(0)).await?; |
There was a problem hiding this comment.
This is a bit concerning to me, why unhandled rejections are not caught by the previous line that dispatch the unload event? Does that mean that we need to update all callsites in the whole codebase to now catch these rejections properly?
There was a problem hiding this comment.
I am currently separating out the functionality of our "polling" event-loop driving code (ie: poll_event_loop) and our "immediate" event-loop driving code (call_and_await, resolve_promise). Unhandled promise rejections are a concern of the "polling" mode of operation, as any number of async tasks may be raised by immediate calls.
Most of this code works only by luck, and only for rejections handled before the function returns. For us to ensure that all rejections pass through the same common unhandled rejection paths, we need to process rejections outside of all immediate APIs and process them entirely through the "polling" interface.
After investigation, it turns out that bench and test are the only places in Deno where we drive the event loop in immediate mode.
| /// Run the event loop up to a given duration. If the runtime resolves early, returns | ||
| /// early. Will always poll the runtime at least once. | ||
| pub async fn run_up_to_duration( | ||
| &mut self, |
There was a problem hiding this comment.
Cool, we should eventually upstream it to deno_core 👍
Extracting some refactorings for the module work that will land in denoland/deno_core#359