From e63aae57ef4a82ce5403f5361d2b94aa35ced337 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Wed, 20 Nov 2024 10:11:00 +0100 Subject: [PATCH] Revert nowAbsolute, add regression test Signed-off-by: Matteo Collina --- benchmarks/timers/now-absolute.mjs | 13 ------- lib/cache/memory-cache-store.js | 3 +- lib/handler/cache-handler.js | 5 +-- lib/interceptor/cache.js | 6 +-- lib/util/timers.js | 20 +--------- test/cache-interceptor/cache-stores.js | 31 ++++++++-------- test/interceptors/cache-fastimers-fix.js | 47 ++++++++++++++++++++++++ test/interceptors/cache.js | 2 - test/timers.js | 12 ------ 9 files changed, 69 insertions(+), 70 deletions(-) delete mode 100644 benchmarks/timers/now-absolute.mjs create mode 100644 test/interceptors/cache-fastimers-fix.js diff --git a/benchmarks/timers/now-absolute.mjs b/benchmarks/timers/now-absolute.mjs deleted file mode 100644 index ef58363db50..00000000000 --- a/benchmarks/timers/now-absolute.mjs +++ /dev/null @@ -1,13 +0,0 @@ -import { bench, group, run } from 'mitata' -import timers from '../../lib/util/timers.js' - -group(() => { - bench('Date.now()', () => { - Date.now() - }) - bench('nowAbsolute()', () => { - timers.nowAbsolute() - }) -}) - -await run() diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 4579c41cc15..6905b66012d 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -1,7 +1,6 @@ 'use strict' const { Writable } = require('node:stream') -const { nowAbsolute } = require('../util/timers.js') /** * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheKey} CacheKey @@ -77,7 +76,7 @@ class MemoryCacheStore { const topLevelKey = `${key.origin}:${key.path}` - const now = nowAbsolute() + const now = Date.now() const entry = this.#entries.get(topLevelKey)?.find((entry) => ( entry.deleteAt > now && entry.method === key.method && diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 1afc1b98347..4e9e2899296 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -7,7 +7,6 @@ const { parseVaryHeader, isEtagUsable } = require('../util/cache') -const { nowAbsolute } = require('../util/timers.js') function noop () {} @@ -123,7 +122,7 @@ class CacheHandler extends DecoratorHandler { return downstreamOnHeaders() } - const now = nowAbsolute() + const now = Date.now() const staleAt = determineStaleAt(now, headers, cacheControlDirectives) if (staleAt) { const varyDirectives = this.#cacheKey.headers && headers.vary @@ -311,7 +310,7 @@ function determineStaleAt (now, headers, cacheControlDirectives) { // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.3 const expiresDate = new Date(headers.expire) if (expiresDate instanceof Date && !isNaN(expiresDate)) { - return now + (nowAbsolute() - expiresDate.getTime()) + return now + (Date.now() - expiresDate.getTime()) } } diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 20e4a2b9219..90c92300844 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -7,7 +7,6 @@ const CacheHandler = require('../handler/cache-handler') const MemoryCacheStore = require('../cache/memory-cache-store') const CacheRevalidationHandler = require('../handler/cache-revalidation-handler') const { assertCacheStore, assertCacheMethods, makeCacheKey, parseCacheControlHeader } = require('../util/cache.js') -const { nowAbsolute } = require('../util/timers.js') const AGE_HEADER = Buffer.from('age') @@ -56,7 +55,7 @@ function needsRevalidation (result, age, cacheControlDirectives) { return true } - const now = nowAbsolute() + const now = Date.now() if (now > result.staleAt) { // Response is stale if (cacheControlDirectives?.['max-stale']) { @@ -186,6 +185,7 @@ module.exports = (opts = {}) => { if (typeof handler.onHeaders === 'function') { // Add the age header // https://www.rfc-editor.org/rfc/rfc9111.html#name-age + const age = Math.round((Date.now() - result.cachedAt) / 1000) // TODO (fix): What if rawHeaders already contains age header? rawHeaders = [...rawHeaders, AGE_HEADER, Buffer.from(`${age}`)] @@ -216,7 +216,7 @@ module.exports = (opts = {}) => { throw new Error('stream is undefined but method isn\'t HEAD') } - const age = Math.round((nowAbsolute() - result.cachedAt) / 1000) + const age = Math.round((Date.now() - result.cachedAt) / 1000) if (requestCacheControl?.['max-age'] && age >= requestCacheControl['max-age']) { // Response is considered expired for this specific request // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.1.1 diff --git a/lib/util/timers.js b/lib/util/timers.js index c948d57b067..c15bbc37aa1 100644 --- a/lib/util/timers.js +++ b/lib/util/timers.js @@ -21,13 +21,6 @@ */ let fastNow = 0 -/** - * The fastNowAbsolute variable contains the rough result of Date.now() - * - * @type {number} - */ -let fastNowAbsolute = Date.now() - /** * RESOLUTION_MS represents the target resolution time in milliseconds. * @@ -129,8 +122,6 @@ function onTick () { */ fastNow += TICK_MS - fastNowAbsolute = Date.now() - /** * The `idx` variable is used to iterate over the `fastTimers` array. * Expired timers are removed by replacing them with the last element in the array. @@ -399,9 +390,6 @@ module.exports = { now () { return fastNow }, - nowAbsolute () { - return fastNowAbsolute - }, /** * Trigger the onTick function to process the fastTimers array. * Exported for testing purposes only. @@ -431,11 +419,5 @@ module.exports = { * Marking as deprecated to discourage any use outside of testing. * @deprecated */ - kFastTimer, - /** - * Exporting for testing purposes only. - * Marking as deprecated to discourage any use outside of testing. - * @deprecated - */ - RESOLUTION_MS + kFastTimer } diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index 0ac97fa72e0..9f67d67bef1 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -5,7 +5,6 @@ const { deepStrictEqual, notEqual, equal } = require('node:assert') const { Readable } = require('node:stream') const { once } = require('node:events') const MemoryCacheStore = require('../../lib/cache/memory-cache-store') -const { nowAbsolute } = require('../../lib/util/timers.js') cacheStoreTests(MemoryCacheStore) @@ -33,9 +32,9 @@ function cacheStoreTests (CacheStore) { statusCode: 200, statusMessage: '', rawHeaders: [Buffer.from('1'), Buffer.from('2'), Buffer.from('3')], - cachedAt: nowAbsolute(), - staleAt: nowAbsolute() + 10000, - deleteAt: nowAbsolute() + 20000 + cachedAt: Date.now(), + staleAt: Date.now() + 10000, + deleteAt: Date.now() + 20000 } const requestBody = ['asd', '123'] @@ -73,9 +72,9 @@ function cacheStoreTests (CacheStore) { statusCode: 200, statusMessage: '', rawHeaders: [Buffer.from('1'), Buffer.from('2'), Buffer.from('3')], - cachedAt: nowAbsolute(), - staleAt: nowAbsolute() + 10000, - deleteAt: nowAbsolute() + 20000 + cachedAt: Date.now(), + staleAt: Date.now() + 10000, + deleteAt: Date.now() + 20000 } const anotherBody = ['asd2', '1234'] @@ -111,9 +110,9 @@ function cacheStoreTests (CacheStore) { statusCode: 200, statusMessage: '', rawHeaders: [Buffer.from('1'), Buffer.from('2'), Buffer.from('3')], - cachedAt: nowAbsolute() - 10000, - staleAt: nowAbsolute() - 1, - deleteAt: nowAbsolute() + 20000 + cachedAt: Date.now() - 10000, + staleAt: Date.now() - 1, + deleteAt: Date.now() + 20000 } const requestBody = ['part1', 'part2'] @@ -144,9 +143,9 @@ function cacheStoreTests (CacheStore) { const requestValue = { statusCode: 200, statusMessage: '', - cachedAt: nowAbsolute() - 20000, - staleAt: nowAbsolute() - 10000, - deleteAt: nowAbsolute() - 5 + cachedAt: Date.now() - 20000, + staleAt: Date.now() - 10000, + deleteAt: Date.now() - 5 } const requestBody = ['part1', 'part2'] @@ -178,9 +177,9 @@ function cacheStoreTests (CacheStore) { vary: { 'some-header': 'hello world' }, - cachedAt: nowAbsolute(), - staleAt: nowAbsolute() + 10000, - deleteAt: nowAbsolute() + 20000 + cachedAt: Date.now(), + staleAt: Date.now() + 10000, + deleteAt: Date.now() + 20000 } const requestBody = ['part1', 'part2'] diff --git a/test/interceptors/cache-fastimers-fix.js b/test/interceptors/cache-fastimers-fix.js new file mode 100644 index 00000000000..65650ceefc4 --- /dev/null +++ b/test/interceptors/cache-fastimers-fix.js @@ -0,0 +1,47 @@ +'use strict' + +const { test, after } = require('node:test') +const { strictEqual } = require('node:assert') +const { createServer } = require('node:http') +const { once } = require('node:events') +const { request, Client, interceptors } = require('../../index') +const { setTimeout: sleep } = require('timers/promises') + +test('revalidates the request when the response is stale', async () => { + let count = 0 + const server = createServer((req, res) => { + res.setHeader('Cache-Control', 'public, max-age=1') + res.end('hello world ' + count++) + }) + + server.listen(0) + await once(server, 'listening') + + const dispatcher = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + server.close() + await dispatcher.close() + }) + + const url = `http://localhost:${server.address().port}` + + { + const res = await request(url, { dispatcher }) + strictEqual(await res.body.text(), 'hello world 0') + } + + { + const res = await request(url, { dispatcher }) + strictEqual(await res.body.text(), 'hello world 0') + } + + await sleep(1000) + + { + const res = await request(url, { dispatcher }) + + strictEqual(await res.body.text(), 'hello world 1') + } +}) diff --git a/test/interceptors/cache.js b/test/interceptors/cache.js index f3084fdc51b..05432a303e9 100644 --- a/test/interceptors/cache.js +++ b/test/interceptors/cache.js @@ -161,7 +161,6 @@ describe('Cache Interceptor', () => { const clock = FakeTimers.install({ shouldClearNativeTimers: true }) - tick(0) const server = createServer((req, res) => { res.setHeader('cache-control', 'public, s-maxage=1, stale-while-revalidate=10') @@ -207,7 +206,6 @@ describe('Cache Interceptor', () => { strictEqual(await response.body.text(), 'asd') clock.tick(1500) - tick(1500) // Now we send two more requests. Both of these should reach the origin, // but now with a conditional header asking if the resource has been diff --git a/test/timers.js b/test/timers.js index b067e61f5e6..9d8cd596e90 100644 --- a/test/timers.js +++ b/test/timers.js @@ -255,16 +255,4 @@ describe('timers', () => { await t.completed }) - - test('nowAbsolute', (t) => { - t = tspl(t, { plan: 1 }) - - const actualNow = Date.now() - - const lowerBound = actualNow - timers.RESOLUTION_MS - const upperBound = actualNow + timers.RESOLUTION_MS - const fastNowAbsolute = timers.nowAbsolute() - - t.equal(fastNowAbsolute >= lowerBound && fastNowAbsolute <= upperBound, true) - }) })