Maximum value allowed for ttl

[paulovieira]

In the set method there is a check to verify that ttl is less than or equal to Math.pow(2, 31). Since this number represents milliseconds, it allows the cached value to be valid only up to about 24 days, which is not that much.

Javascript numbers can go up to Math.pow(2, 53) - 1:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER

Is there any reason to not use this constant instead?

[Marsup]

setTimeout doesn't handle more than that. Could always use big-time instead.

[paulovieira]

What about calling drop directly in get (if Date.now() - envelope.stored > envelope.ttl), instead of using the setTimeout in set?

Advantages:

• would allow ttl bigger than 2^32
• would avoid scheduling a callback for each cached value, which should improve the required memory

Disadvantages:

• would be duplicating the login in client.js

[Marsup]

If you never call get it never gets cleaned, not what I'd expect.

[paulovieira]

Yeah, that's another one for the disadvantages. But it reminds of the saying:

"If a tree falls in a forest and no one is around to hear it, does it make a sound?" :smile:

If get is never called then that value is never needed. Should we care about having stalled values in memory?

The downside is the amount of memory those values would be taking.

This could be solved with a setInterval which would periodically clean the stalled values (say every 60 seconds). Sounds a bit hacky though...

[Marsup]

Why are you fighting the big-time solution ? This is good as it is, just need a better timeout.

[paulovieira]

You're right, I didn't read well the initial response (didn't know about the big-time module).

[Marsup]

Don't close it just yet, that's a problem @cjihrig might want to fix.

[cjihrig]

Moving my comments from #37 over here since that PR landed.

I'm open to using big-time. I actually tried this before in #29, but @hueniverse said no because it required babel (rest params). However, this is no longer the case in Node 6.

FYI - big-time is now updated to drop Babel as of continuationlabs/big-time#15

[blakeembrey]

Ah, this issue just bit me too. I didn't actually care about the expiration overall, but the fact I was using it to test HTTP caching of dates with a year meant my tests were failing. Would you be interested in a PR that periodically prunes the timeouts on an interval instead of setting individual timeouts?

[hueniverse]

Should be resolved by #8.

@blakeembrey I'll take a PR.

[lock[bot]]

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.