bugfix: execute operations sequentially to guarantee key exists when …

[0xD3ADBE3F]

the set operation has to be fulfilled first, in order to set a expire operation on the key (you can't set a expire on a non existing key). Promise.all is not sequential.

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 90ad1ac2086f27ea98ea04686a1d9714c58a42d2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@neshca/cache-handler	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[better-salmon]

Hey @mdahiemstra, the node-redis package utilizes a command queue, so there's no need to execute this code sequentially.

[0xD3ADBE3F]

Thanks for your reply! Ah yes I assumed that, but AFAIK this queue does not sort commands based on dependency right? So the SET command has to be executed before the EXPIRE command?

Or is this being done another way? I found out there were a lot of keys in my redis db without a ttl value, I reasoned it to this part of the code, am I wrong? Regards

[better-salmon]

this queue does not sort commands based on dependency right?

The only thing that matters is the order of the command calls.

The part of the packages/cache-handler/src/handlers/redis-stack.ts file:

const setCacheValue = client.json.set(
  options,
  keyPrefix + key,
  ".",
  cacheHandlerValue as unknown as RedisJSON
);

const expireCacheValue = cacheHandlerValue.lifespan
  ? client.expireAt(
      options,
      keyPrefix + key,
      cacheHandlerValue.lifespan.expireAt
    )
  : undefined;

await Promise.all([setCacheValue, expireCacheValue]);

First, the set command is called, which internally triggers the #sendCommand method in the RedisClient class. This action creates a new promise and adds it to the queue. Then, the expireAt command is called, which also internally triggers #sendCommand, creating another promise and adding it to the queue.

Therefore, the queue will appear as follows: expireAt -> set -> queue exit

Since the queue operates as a FIFO (first in, first out) structure, the first command will always be executed first.

---

I found out there were a lot of keys in my redis db without a ttl value

You may have cache entries without an expire time only if the cacheHandlerValue.lifespan is null. And this is the case when you are using the fallback: false option in the getStaticProps function. See the documentation.

[0xD3ADBE3F]

I understand now, thank you for your time and explanation. I will close the PR.

[steevsachs]

@better-salmon I realize this PR is closed, but I just encountered an issue that may be related to this, so wanted to call it out here.

In your explanation, you imply that the sequence of calls in the Promise.all is guaranteed. However, Promise.all just awaits all of those promises, and does not guarantee the order of their resolution.

If client.set contains async prior to dispatching the SET call to redis, it is possible for the EXPIREAT call to be dispatched before the SET has been dispatched.

I believe this is a simple demonstration of this occurring. https://codesandbox.io/p/sandbox/gqsths?file=%2Fsrc%2Fdemo.ts

Is there something about a practical implementation that logically prevents this from occurring? It seems possible that the present cache handler implementation is making assumptions about the internal implementation of the redis client.

Please let me know if a new PR would be helpful to address this.