fix(SimpleIdentifyThrottler): don't sleep negative amounts (#10669)

* fix(SimpleIdentifyThrottler): don't sleep negative amounts

* fix: test
This commit is contained in:
Denis-Adrian Cristea
2024-12-31 20:16:57 +02:00
committed by GitHub
parent b81ad113a0
commit b854c7b979
2 changed files with 37 additions and 12 deletions

View File

@@ -1,33 +1,58 @@
// @ts-nocheck
import { setTimeout as sleep } from 'node:timers/promises';
import { expect, test, vi } from 'vitest';
import * as timers from 'node:timers/promises';
import { expect, test, vi, beforeEach, afterEach } from 'vitest';
import { SimpleIdentifyThrottler } from '../../src/index.js';
vi.mock('node:timers/promises', () => ({
setTimeout: vi.fn(),
}));
const throttler = new SimpleIdentifyThrottler(2);
let throttler: SimpleIdentifyThrottler;
const controller = new AbortController();
vi.useFakeTimers();
const NOW = vi.fn().mockReturnValue(Date.now());
const TIME = Date.now();
const NOW = vi.fn().mockReturnValue(TIME);
global.Date.now = NOW;
const sleep = vi.spyOn(timers, 'setTimeout');
beforeEach(() => {
throttler = new SimpleIdentifyThrottler(2);
});
afterEach(() => {
sleep.mockClear();
});
test('basic case', async () => {
// Those shouldn't wait since they're in different keys
await throttler.waitForIdentify(0);
await throttler.waitForIdentify(0, controller.signal);
expect(sleep).not.toHaveBeenCalled();
await throttler.waitForIdentify(1);
await throttler.waitForIdentify(1, controller.signal);
expect(sleep).not.toHaveBeenCalled();
// Those should wait
await throttler.waitForIdentify(2);
await throttler.waitForIdentify(2, controller.signal);
expect(sleep).toHaveBeenCalledTimes(1);
await throttler.waitForIdentify(3);
await throttler.waitForIdentify(3, controller.signal);
expect(sleep).toHaveBeenCalledTimes(2);
});
test('does not call sleep with a negative time', async () => {
await throttler.waitForIdentify(0, controller.signal);
expect(sleep).not.toHaveBeenCalled();
await throttler.waitForIdentify(0, controller.signal);
expect(sleep).toHaveBeenCalledTimes(1);
// By overshooting, we're simulating a bug that existed prior to this test, where-in by enough time
// passing before the shard tried to identify for a subsequent time, the passed value would end up being negative
// (and this was unchecked). Node simply treats that as 1ms, so it wasn't particularly harmful, but they
// recently introduced a warning for it, so we want to avoid that.
NOW.mockReturnValueOnce(TIME + 10_000);
await throttler.waitForIdentify(0, controller.signal);
expect(sleep).toHaveBeenCalledTimes(1);
});

View File

@@ -36,7 +36,7 @@ export class SimpleIdentifyThrottler implements IIdentifyThrottler {
try {
const diff = state.resetsAt - Date.now();
if (diff <= 5_000) {
if (diff > 0 && diff <= 5_000) {
// To account for the latency the IDENTIFY payload goes through, we add a bit more wait time
const time = diff + Math.random() * 1_500;
await sleep(time);