-
Notifications
You must be signed in to change notification settings - Fork 5k
bun:test: keep built-in modules' own timers out of fake timers (internal/timers) #37987
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| // — to convert at every assignment and risked silent layout drift). | ||
| use Timespec as timespec; | ||
| pub use bun_core::Timespec; | ||
| use bun_core::TimespecMockMode; | ||
|
|
||
| // Re-export so higher tiers see the *same* type they pass to | ||
| // `bun_io::heap::Intrusive<EventLoopTimer, _>` (a zero-sized local stub | ||
|
|
@@ -179,6 +180,11 @@ impl EventLoopTimer { | |
| #[derive(Copy, Clone, Eq, PartialEq, strum::IntoStaticStr)] | ||
| pub enum Tag { | ||
| TimeoutObject, | ||
| /// A `TimeoutObject` scheduled by a built-in JS module through | ||
| /// `internal/timers` (socket idle timeouts, `child_process` kill timers, | ||
| /// ...): the same container as `TimeoutObject`, but a runtime-internal | ||
| /// timeout as far as `allow_fake_timers` is concerned. | ||
| InternalTimeoutObject, | ||
| ImmediateObject, | ||
| StatWatcherScheduler, | ||
| UpgradedDuplex, | ||
|
|
@@ -217,6 +223,18 @@ impl Tag { | |
| Tag::TimeoutObject | Tag::AbortSignalTimeout | Tag::CronJob | ||
| ) | ||
| } | ||
|
|
||
| /// The clock an owner with this tag arms with (the rule stated on | ||
| /// [`Self::allow_fake_timers`]), for owners that exist under more than one | ||
| /// tag: a real-heap timer armed from the mocked clock is due immediately, | ||
| /// and re-arms due immediately. | ||
|
Comment on lines
+227
to
+230
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub fn clock(self) -> TimespecMockMode { | ||
| if self.allow_fake_timers() { | ||
| TimespecMockMode::AllowMockedTime | ||
| } else { | ||
| TimespecMockMode::ForceRealTime | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Stamp out one `unsafe fn $method(*const EventLoopTimer) -> *mut Self` per | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -557,6 +557,9 @@ export const arrayBufferViewHasBuffer = $newCppFunction( | |
|
|
||
| export const timerInternals = { | ||
| timerClockMs: $newRustFunction("runtime/timer/Timer.rs", "internal_bindings.timerClockMs", 0), | ||
| // The timers built-in modules schedule their own deadlines with; unlike the | ||
| // globals they are not touched by jest.useFakeTimers(). | ||
|
Comment on lines
+560
to
+561
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| internalTimers: require("internal/timers"), | ||
| }; | ||
|
|
||
| // Raw datagram descriptor helpers for tests that need an unbound fd (which | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,20 @@ const NumberIsFinite = Number.isFinite; | |
|
|
||
| const TIMEOUT_MAX = 2 ** 31 - 1; | ||
|
|
||
| // Timers for the runtime's own deadlines (socket idle timeouts, listen() | ||
| // callbacks, child_process kill timers, ...). The globals belong to user code: | ||
| // jest.useFakeTimers() freezes, counts, advances and clears every timer created | ||
| // through them, and user code may replace them outright. These create the same | ||
| // Timeout objects but never take part in fake timers, like the private timer | ||
| // references Node's lib/ uses. Built-in modules take all four from here | ||
| // (test/internal/source-lints/builtin-timer-globals.test.ts); the global | ||
| // clearTimeout would clear these too, the private one just stays out of reach | ||
| // of replaced globals. | ||
|
Comment on lines
+7
to
+15
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| const setTimeout = $newCppFunction("node/NodeTimers.cpp", "functionSetTimeoutInternal", 1); | ||
| const setInterval = $newCppFunction("node/NodeTimers.cpp", "functionSetIntervalInternal", 1); | ||
| const clearTimeout = $newCppFunction("node/NodeTimers.cpp", "functionClearTimeout", 1); | ||
| const clearInterval = $newCppFunction("node/NodeTimers.cpp", "functionClearInterval", 1); | ||
|
|
||
| function getTimerDuration(msecs, name) { | ||
| validateNumber(msecs, name); | ||
| if (msecs < 0 || !NumberIsFinite(msecs)) { | ||
|
|
@@ -29,4 +43,8 @@ export default { | |
| // tests that inspect socket[kTimeout]. | ||
| kTimeout: Symbol.for("::buntimeout::"), | ||
| getTimerDuration, | ||
| setTimeout, | ||
| setInterval, | ||
| clearTimeout, | ||
| clearInterval, | ||
| }; | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,23 +2,29 @@ | |
|
|
||
| #include "ErrorCode.h" | ||
| #include "headers.h" | ||
| #include <wtf/text/MakeString.h> | ||
|
|
||
| namespace Bun { | ||
|
|
||
| using namespace JSC; | ||
|
|
||
| JSC_DEFINE_HOST_FUNCTION(functionSetTimeout, | ||
| (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) | ||
| using TimerScheduler = JSC::EncodedJSValue (*)(JSC::JSGlobalObject*, JSC::EncodedJSValue callback, JSC::EncodedJSValue arguments, JSC::EncodedJSValue countdown); | ||
|
|
||
| // setTimeout(callback, delay, ...args) / setInterval(callback, delay, ...args). | ||
| // The extra arguments are packed the way Bun__JSTimeout__call (NodeTimerObject.cpp) | ||
| // unpacks them: undefined for none, the value itself for one, a JSCellButterfly | ||
| // for several. | ||
|
Comment on lines
+13
to
+16
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| static JSC::EncodedJSValue scheduleTimer(JSC::JSGlobalObject* globalObject, JSC::CallFrame* callFrame, ASCIILiteral name, TimerScheduler schedule) | ||
| { | ||
| auto& vm = JSC::getVM(globalObject); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| JSC::JSValue job = callFrame->argument(0); | ||
| JSC::JSValue num = callFrame->argument(1); | ||
| JSC::JSValue arguments = jsUndefined(); | ||
| size_t argumentCount = callFrame->argumentCount(); | ||
| auto scope = DECLARE_THROW_SCOPE(globalObject->vm()); | ||
| switch (argumentCount) { | ||
|
|
||
| switch (callFrame->argumentCount()) { | ||
| case 0: { | ||
| Bun::throwError(globalObject, scope, ErrorCode::ERR_INVALID_ARG_TYPE, "setTimeout requires 1 argument (a function)"_s); | ||
| Bun::throwError(globalObject, scope, ErrorCode::ERR_INVALID_ARG_TYPE, makeString(name, " requires 1 argument (a function)"_s)); | ||
| return {}; | ||
| } | ||
| case 1: | ||
|
|
@@ -44,7 +50,7 @@ JSC_DEFINE_HOST_FUNCTION(functionSetTimeout, | |
| } | ||
|
|
||
| if (!job.isObject() || !job.getObject()->isCallable()) [[unlikely]] { | ||
| Bun::throwError(globalObject, scope, ErrorCode::ERR_INVALID_ARG_TYPE, "setTimeout expects a function"_s); | ||
| Bun::throwError(globalObject, scope, ErrorCode::ERR_INVALID_ARG_TYPE, makeString(name, " expects a function"_s)); | ||
| return {}; | ||
| } | ||
|
|
||
|
|
@@ -60,64 +66,36 @@ JSC_DEFINE_HOST_FUNCTION(functionSetTimeout, | |
| } | ||
| #endif | ||
|
|
||
| return Bun__Timer__setTimeout(globalObject, JSC::JSValue::encode(job), JSC::JSValue::encode(arguments), JSValue::encode(num)); | ||
| RELEASE_AND_RETURN(scope, schedule(globalObject, JSC::JSValue::encode(job), JSC::JSValue::encode(arguments), JSC::JSValue::encode(num))); | ||
| } | ||
|
|
||
| JSC_DEFINE_HOST_FUNCTION(functionSetInterval, | ||
| JSC_DEFINE_HOST_FUNCTION(functionSetTimeout, | ||
| (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) | ||
| { | ||
| auto& vm = JSC::getVM(globalObject); | ||
| JSC::JSValue job = callFrame->argument(0); | ||
| JSC::JSValue num = callFrame->argument(1); | ||
| JSC::JSValue arguments = jsUndefined(); | ||
| size_t argumentCount = callFrame->argumentCount(); | ||
| auto scope = DECLARE_THROW_SCOPE(globalObject->vm()); | ||
|
|
||
| switch (argumentCount) { | ||
| case 0: { | ||
| Bun::throwError(globalObject, scope, ErrorCode::ERR_INVALID_ARG_TYPE, "setInterval requires 1 argument (a function)"_s); | ||
| return {}; | ||
| } | ||
| case 1: | ||
| case 2: { | ||
| break; | ||
| } | ||
| case 3: { | ||
| arguments = callFrame->argument(2); | ||
| break; | ||
| } | ||
|
|
||
| default: { | ||
| ArgList argumentsList = ArgList(callFrame, 2); | ||
| auto* args = JSC::JSCellButterfly::tryCreateFromArgList(vm, argumentsList); | ||
|
|
||
| if (!args) [[unlikely]] { | ||
| JSC::throwOutOfMemoryError(globalObject, scope); | ||
| return {}; | ||
| } | ||
|
|
||
| arguments = JSValue(args); | ||
| } | ||
| } | ||
| return scheduleTimer(globalObject, callFrame, "setTimeout"_s, Bun__Timer__setTimeout); | ||
| } | ||
|
|
||
| if (!job.isObject() || !job.getObject()->isCallable()) [[unlikely]] { | ||
| Bun::throwError(globalObject, scope, ErrorCode::ERR_INVALID_ARG_TYPE, "setInterval expects a function"_s); | ||
| return {}; | ||
| } | ||
| JSC_DEFINE_HOST_FUNCTION(functionSetInterval, | ||
| (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) | ||
| { | ||
| return scheduleTimer(globalObject, callFrame, "setInterval"_s, Bun__Timer__setInterval); | ||
| } | ||
|
|
||
| #ifdef BUN_DEBUG | ||
| /** View the file name of the JS file that called this function | ||
| * from a debugger */ | ||
| SourceOrigin sourceOrigin = callFrame->callerSourceOrigin(vm); | ||
| auto fileNameUTF8 = sourceOrigin.string().utf8(); | ||
| const char* fileName = fileNameUTF8.data(); | ||
| static const char* lastFileName = nullptr; | ||
| if (lastFileName != fileName) { | ||
| lastFileName = fileName; | ||
| } | ||
| #endif | ||
| // The setTimeout/setInterval that built-in JS modules schedule their own | ||
| // deadlines with (src/js/internal/timers.ts). Same arguments and same Timeout | ||
| // object as the globals, but the timer is never handed to bun:test's fake | ||
| // timers, so socket timeouts, listen() callbacks and the like keep working | ||
| // while a test has jest.useFakeTimers() active. | ||
|
Comment on lines
+84
to
+88
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| JSC_DEFINE_HOST_FUNCTION(functionSetTimeoutInternal, | ||
| (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) | ||
| { | ||
| return scheduleTimer(globalObject, callFrame, "setTimeout"_s, Bun__Timer__setTimeoutInternal); | ||
| } | ||
|
|
||
| return Bun__Timer__setInterval(globalObject, JSC::JSValue::encode(job), JSC::JSValue::encode(arguments), JSValue::encode(num)); | ||
| JSC_DEFINE_HOST_FUNCTION(functionSetIntervalInternal, | ||
| (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) | ||
| { | ||
| return scheduleTimer(globalObject, callFrame, "setInterval"_s, Bun__Timer__setIntervalInternal); | ||
| } | ||
|
|
||
| // https://developer.mozilla.org/en-US/docs/Web/API/Window/setImmediate | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code