kornelski / http-cache-semantics

RFC 7234 in JavaScript. Parses HTTP headers to correctly compute cacheability of responses, even in complex cases
http://httpwg.org/specs/rfc7234.html
BSD 2-Clause "Simplified" License
244 stars 27 forks source link

timeToLive() returns floats #35

Closed avivimgn closed 10 months ago

avivimgn commented 3 years ago

Not always of course, but in some cases timeToLive() returns floats. Which makes sense since in maxAge() there's a division by 1000 and then later a multiplication by 1000 done by timeToLive(), yet the code ignores computers' struggle with float arithmetic. Since the doc mentions timeToLive() returns time in milliseconds, one would expect it to return an integer.

Math.round() in the correct spot or some other solution would be appreciated, let me know what you think.

kornelski commented 3 years ago

Does this break anything other than expectations?

Milliseconds can be fractional too.

avivimgn commented 3 years ago

It causes a problem in https://github.com/lukechilds/cacheable-request. Since this is the root cause of the problem, I opened the issue here. Although milliseconds can have fractions, the fractions in this case are not caused by the division between the numbers, but by float arithmetic problems, it seems to me.

That's the error I'm getting in cacheable-request which is coming from Keyv I think: Error [CacheError]: ERR value is not an integer or out of range

code snippet in cacheable-request:

let ttl = opts.strictTtl ? response.cachePolicy.timeToLive() : undefined;
if (opts.maxTtl) {
    ttl = ttl ? Math.min(ttl, opts.maxTtl) : opts.maxTtl;
}

await this.cache.set(key, value, ttl);

The program gets to this.cache.set() providing ttl = response.cachePolicy.timeToLive() = 1256996.9999999998 for example, and once set() is executed, the error above is thrown.

In order to trigger this behavior I'm setting private,max-age=4 on cache-control header in the request, max-age 4 can be changed to any other number that triggers this behavior.

avivimgn commented 3 years ago

If you think this behavior is correct, it's fine, I'll just open an issue in cacheable-request. let me know.