refc!: use new store, cleanup old files #7
Conversation
|
Thank you for doing this! I've started to look through it, but I'm not going to be able to finish reviewing tonight. Hopefully I can find some time this weekend. |
source/store.ts
Outdated
| let record = await new Promise<ClientRecord>((resolve, reject) => { | ||
| this.client.get(key, (error, data) => { | ||
| if (error) { | ||
| reject(error) | ||
| return | ||
| } | ||
|
|
||
| resolve(data as ClientRecord) | ||
| }) | ||
| }) | ||
|
|
||
| if (record === undefined && delta < 0) { | ||
| // If the record does not exist, and we are supposed to decrement the key, | ||
| // we don't need to do anything, so we return. | ||
| return | ||
| } | ||
|
|
||
| if (record === undefined) { | ||
| // If a record does not exist, and must be incremented, then add a record | ||
| // for that key. | ||
| record = await new Promise<ClientRecord>((resolve, reject) => { | ||
| const payload: ClientRecord = { hits: 1, time: Date.now() } | ||
|
|
||
| this.client.add(key, payload, this.expiration, (error) => { | ||
| if (error) { | ||
| reject(error) | ||
| return | ||
| } | ||
|
|
||
| resolve(payload) | ||
| }) | ||
| }) | ||
| } else { | ||
| // If it does exist, simply change the hit count by `delta`. | ||
| record = await new Promise<ClientRecord>((resolve, reject) => { | ||
| const payload: ClientRecord = { ...record, hits: record.hits + delta } | ||
| const expiration = Math.floor( | ||
| // = window - (time elapsed since start of window) [in seconds] | ||
| this.expiration - (Date.now() - payload.time) / 1000, | ||
| ) | ||
|
|
||
| this.client.replace(key, payload, expiration, (error) => { | ||
| if (error) { | ||
| reject(error) | ||
| return | ||
| } | ||
|
|
||
| resolve(payload) | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
I liked the old flow of calling incr/decr. Calling get then create/update leaves it you open to race conditions where a bunch of requests come in at once, and all get the initial value then all overwrite eachother's additions.
It's also fewer requests in the common case of a returning visitor.
IMO, removing this function and just letting increment and decrement each have their own logic probably makes more sense here.
There was a problem hiding this comment.
We could do that, but then we won't be able to return an accurate resetTime to the middleware.
There was a problem hiding this comment.
Maybe I should use the cas command instead of replace to avoid a race condition?
There was a problem hiding this comment.
Oh, that's a good point. I should familiarize myself with memcahed's commands.
There was a problem hiding this comment.
What about incr followed by either creating the key if it's not found, or mg (meta get), with the t to return the expiration time in seconds if the incr worked.
It's still two commands, but there's no risk of collision (after the initial creation) this way.
There was a problem hiding this comment.
oh, snap, the Meta Arithmetic command, ma is exactly what we want.
ma mykey N60 v t will:
- create mykey if it doesn't exist with a expiration time of 60 seconds (
N60) - increment it (
MI, which is the default so I left it out) by 1 (D1, also default) - return the new value (
v) and the expiration time (t)
That's a single memcached request per hit, regardless of weather or not the key already existed.
Similarly, ma mykey N60 MD v t will decrement (MD) it instead of incrementing.
I'm going to see if I can find a memcached client that supports meta commands, and I might send some PRs if I can't find one.
There was a problem hiding this comment.
I just filed a ticket on memcached-client requesting support for meta commands. I'm kind of afraid that I just signed myself up for a ton of work, but I think it will make this a better library in the long run.
There was a problem hiding this comment.
oh, snap, the Meta Arithmetic command, ma is exactly what we want.
It is!! Great discovery!
Thanks for opening the issue, I'll do my best to help implement it.
There was a problem hiding this comment.
I don't see any reply on the issue.. should we try pinging the maintainers?
Otherwise we could try using the private .command method on the library we are using right now.
|
Thanks for the review comments, I'll address them by tomorrow! |
|
Made the suggested changes:
|
|
This should be ready to go! |
|
Awesome, I'll take another look sometime soon |
readme.md
Outdated
| > [pure `esm` package](https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c). | ||
| > This means you cannot `require` it in `cjs` projects anymore. Please see the | ||
| > linked article for more information. |
There was a problem hiding this comment.
We should delete this text also.
|
I think I addressed all the review comments, let me know if I should change anything else. |
Storeinterface fromexpress-rate-limitv6 #5 by implementing the new store interface.Converts the package to pure ESM.Exports both ESM and CJS files.