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

Revert nowAbsolute, add regression test #3850

Merged
merged 1 commit into from
Nov 20, 2024
Merged
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
13 changes: 0 additions & 13 deletions benchmarks/timers/now-absolute.mjs

This file was deleted.

3 changes: 1 addition & 2 deletions lib/cache/memory-cache-store.js
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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 &&
Expand Down
5 changes: 2 additions & 3 deletions lib/handler/cache-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ const {
parseVaryHeader,
isEtagUsable
} = require('../util/cache')
const { nowAbsolute } = require('../util/timers.js')

function noop () {}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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())
}
}

Expand Down
6 changes: 3 additions & 3 deletions lib/interceptor/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand Down Expand Up @@ -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']) {
Expand Down Expand Up @@ -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}`)]
Expand Down Expand Up @@ -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
Expand Down
20 changes: 1 addition & 19 deletions lib/util/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
}
31 changes: 15 additions & 16 deletions test/cache-interceptor/cache-stores.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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']

Expand Down Expand Up @@ -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']

Expand Down Expand Up @@ -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']

Expand Down Expand Up @@ -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']

Expand Down Expand Up @@ -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']

Expand Down
47 changes: 47 additions & 0 deletions test/interceptors/cache-fastimers-fix.js
Original file line number Diff line number Diff line change
@@ -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')
}
})
2 changes: 0 additions & 2 deletions test/interceptors/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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
Expand Down
12 changes: 0 additions & 12 deletions test/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
})
Loading