Skip to content
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

Fix setTimeout ID collisions. #2110

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/workerd/api/global-scope.c++
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,6 @@ TimeoutId::NumberType ServiceWorkerGlobalScope::setTimeoutInternal(
jsg::Function<void()> function,
double msDelay) {
auto timeoutId = IoContext::current().setTimeoutImpl(
timeoutIdGenerator,
/** repeats = */ false,
kj::mv(function),
msDelay);
Expand All @@ -707,7 +706,6 @@ TimeoutId::NumberType ServiceWorkerGlobalScope::setTimeout(
function(js, kj::mv(args));
};
auto timeoutId = IoContext::current().setTimeoutImpl(
timeoutIdGenerator,
/* repeats = */ false,
[function = kj::mv(fn)](jsg::Lock& js) mutable {
function(js);
Expand Down Expand Up @@ -737,7 +735,6 @@ TimeoutId::NumberType ServiceWorkerGlobalScope::setInterval(
function(js, jsg::Arguments(kj::mv(argv)));
};
auto timeoutId = IoContext::current().setTimeoutImpl(
timeoutIdGenerator,
/* repeats = */ true,
[function = kj::mv(fn)](jsg::Lock& js) mutable {
function(js);
Expand Down
2 changes: 1 addition & 1 deletion src/workerd/api/gpu/gpu-async-runner.c++
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ void AsyncRunner::QueueTick() {
tick_queued_ = true;

IoContext::current().setTimeoutImpl(
timeoutIdGenerator, false,
false,
[this](jsg::Lock& js) mutable {
this->tick_queued_ = false;
if (this->count_ > 0) {
Expand Down
1 change: 0 additions & 1 deletion src/workerd/api/gpu/gpu-async-runner.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ class AsyncRunner : public kj::Refcounted {
wgpu::Device const device_;
uint64_t count_ = 0;
bool tick_queued_ = false;
TimeoutId::Generator timeoutIdGenerator;
};

// AsyncTask is a RAII helper for calling AsyncRunner::Begin() on construction,
Expand Down
5 changes: 5 additions & 0 deletions src/workerd/api/gpu/webgpu-buffer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ export class DurableObjectExample {
}

async fetch() {
// Some WebGPU APIs use setTimeout under the hood. There used to be a bug whereby webGPU would
// try to use a timeout ID that was already in use by a call to a JS timer API (or vice versa).
// We set a timeout here as a regression test.
setTimeout(() => { console.log("timeout fired"); }, 30000);

ok(navigator.gpu);
const adapter = await navigator.gpu.requestAdapter();
ok(adapter);
Expand Down
16 changes: 8 additions & 8 deletions src/workerd/io/io-context.c++
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ public:
KJ_DISALLOW_COPY_AND_MOVE(TimeoutManagerImpl);

TimeoutId setTimeout(
IoContext& context, TimeoutId::Generator& generator, TimeoutParameters params) override {
auto [id, it] = addState(generator, kj::mv(params));
IoContext& context, TimeoutParameters params) override {
auto [id, it] = addState(kj::mv(params));
setTimeoutImpl(context, it);
return id;
}
Expand All @@ -56,7 +56,7 @@ private:
TimeoutId id;
Iterator it;
};
IdAndIterator addState(TimeoutId::Generator& generator, TimeoutParameters params);
IdAndIterator addState(TimeoutParameters params);

void setTimeoutImpl(IoContext& context, Iterator it);

Expand All @@ -75,6 +75,8 @@ private:
}
};

TimeoutId::Generator timeoutIdGenerator;

// Tracks registered timeouts sorted by the next time the timeout is expected to fire.
//
// The associated fulfiller should be fulfilled when the time has been reached AND all previous
Expand Down Expand Up @@ -591,13 +593,12 @@ void IoContext::TimeoutManagerImpl::TimeoutState::cancel() {
++manager.timeoutsFinished;
}

auto IoContext::TimeoutManagerImpl::addState(
TimeoutId::Generator& generator, TimeoutParameters params) -> IdAndIterator {
auto IoContext::TimeoutManagerImpl::addState(TimeoutParameters params) -> IdAndIterator {
JSG_REQUIRE(getTimeoutCount() < MAX_TIMEOUTS, DOMQuotaExceededError,
"You have exceeded the number of timeouts you may set.",
MAX_TIMEOUTS);

auto id = generator.getNext();
auto id = timeoutIdGenerator.getNext();
auto [it, wasEmplaced] = timeouts.try_emplace(id, *this, kj::mv(params));
if (!wasEmplaced) {
// We shouldn't have reached here because the `TimeoutId::Generator` throws if it reaches
Expand Down Expand Up @@ -763,7 +764,6 @@ void IoContext::TimeoutManagerImpl::clearTimeout(
}

TimeoutId IoContext::setTimeoutImpl(
TimeoutId::Generator& generator,
bool repeat,
jsg::Function<void()> function,
double msDelay) {
Expand All @@ -775,7 +775,7 @@ TimeoutId IoContext::setTimeoutImpl(
msDelay <= 0 || std::isnan(msDelay) ? 0 :
msDelay >= static_cast<double>(max) ? max : static_cast<int64_t>(msDelay);
auto params = TimeoutManager::TimeoutParameters(repeat, delay, kj::mv(function));
return timeoutManager->setTimeout(*this, generator, kj::mv(params));
return timeoutManager->setTimeout(*this, kj::mv(params));
}

void IoContext::clearTimeoutImpl(TimeoutId id) {
Expand Down
1 change: 0 additions & 1 deletion src/workerd/io/io-context.h
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,6 @@ class IoContext final: public kj::Refcounted, private kj::TaskSet::ErrorHandler
// Used to implement setTimeout(). We don't expose the timer directly because the
// promises it returns need to live in this I/O context, anyway.
TimeoutId setTimeoutImpl(
TimeoutId::Generator& generator,
bool repeat,
jsg::Function<void()> function,
double msDelay);
Expand Down
5 changes: 2 additions & 3 deletions src/workerd/io/io-timers.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,10 @@ class TimeoutManager {
kj::Maybe<jsg::Function<void()>> function;
};

virtual TimeoutId setTimeout(
IoContext& context, TimeoutId::Generator& generator, TimeoutParameters params) = 0;
virtual TimeoutId setTimeout(IoContext& context, TimeoutParameters params) = 0;
virtual void clearTimeout(IoContext& context, TimeoutId id) = 0;
virtual size_t getTimeoutCount() const = 0;
virtual kj::Maybe<kj::Date> getNextTimeout() const = 0;
};

} // namespace workerd
} // namespace workerd
Loading