[useResponseCache] Ensure calls to cache are awaited

sachinahya commented 2 months ago

Description

This change ensures that the calls to the supplied cache implementation are awaited and returned, in the case that the methods are async and return promises. This prevents "floating promises" from being created which will crash the Node process if they are rejected.

I believe this is not a breaking change since all supported Node versions have the behaviour of terminating on unhandled promise rejections, and that the behaviour without this change is not intentional or desired.

Fixes #2240.

Type of change

Please delete options that are not relevant.

[x] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Unit tests added for set and invalidate calls to the cache implementation, ensuring that sync errors and promise rejections don't result in process termination.

Test Environment:

OS: macOS 14.4.1
@envelop/response-cache: 6.1.2
NodeJS: v20.11.0

Checklist:

[x] I have followed the CONTRIBUTING doc and the style guidelines of this project
[x] I have performed a self-review of my own code
[x] I have commented on my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
[ ] My changes generate no new warnings
[x] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes
[ ] Any dependent changes have been merged and published in downstream modules

changeset-bot[bot] commented 2 months ago

🦋 Changeset detected

Latest commit: f38e8d53992ac2eeab384c199f05233730e3c056

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

This PR includes changesets to release 1 package

| Name | Type | | ----------------------- | ----- | | @envelop/response-cache | 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

EmrysMyrddin commented 1 month ago

Thank you for you contribution! Very appreciated!

I agree that we should not have floating promises without any error handling here, but I'm not sure that awaiting it is the right approach about this.

My concern is that if there is a set failure, the entire request handling will fail. You probably don't want to fail responding a valid request just because the cache is down, and you probably don't want to add the latency of the cache network travel.

For this reasons, perhaps just adding a .catch(err => logger.warn('Failed to cache response', err)) instead ? Do you think it would suite your needs ?

The drawback is that it opens the possibility to have hanging open handles when shuting down the server, which can prevent server for gracefully exit. That being said, it's already the case with the current implementation. We should probably better add an abort signal to allow canceling these promises on shutdown, but not sure it is worth the added complexity for a case that is probably very rare.

EmrysMyrddin commented 1 month ago

After some discussion with a colleague, it appears that we actually already have a way to handle floating promises like this. There is an undocumented waitUntil function in the yoga context. It's provided by whatwg-node/server, and it's signature is waitUntil(f: Promise<void> | void): void.

sachinahya commented 1 month ago

Adding a catch handler works for us too. I also agree that the case for aborting floating promises is not worth the complexity since I'd imagine it's very rare to have a promise that never resolves.

As far as logging goes, I'm not sure where logger would come from in your example as I can't see where it gets passed in from. Maybe we can add an empty .catch and leave the logging to the implementation of Cache, or add a console.error?

EmrysMyrddin commented 1 month ago

Sorry, I was not very clear. In fact, there is a function in the context called waitUntil that is already doing exactly this :-) And it allows for a better compatibility in ephemeral environment like cloud functions. You can use it instead of manually adding a .catch. Under the hood, if an implementation is not provided by the environment, it calls .allSettled on every promises passed to it as argument.

sachinahya commented 1 month ago

Thanks, could you point us to some documentation for this once it's ready and we'll give it a go?

n1ru4l / envelop