-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: unhandled Promise rejections in pipeline.exec [skip ci] #1466
fix: unhandled Promise rejections in pipeline.exec [skip ci] #1466
Conversation
Deliberately add `asCallback` on the **same** Promise instance being returned, not on a different Promise instance that resolves to the same thing. Avoid this error: ```javascript process.on('unhandledRejection', (reason) => console.log('unhandledRejection', reason)); const x = Promise.reject(new Error()); x.catch((e) => console.log('caught x', e)); const causesUnhandledRejection = Promise.resolve().then(() => x); ``` Related to redis#1466
e2892ad
to
529bed6
Compare
Deliberately add `asCallback` on the **same** Promise instance being returned, not on a different Promise instance that resolves to the same thing. Avoid this error: ```javascript process.on('unhandledRejection', (reason) => console.log('unhandledRejection', reason)); const x = Promise.reject(new Error()); x.catch((e) => console.log('caught x', e)); const causesUnhandledRejection = Promise.resolve().then(() => x); ``` Related to redis#1466
lib/pipeline.ts
Outdated
_this.redis._addedScriptHashes[scripts[i].sha] = true; | ||
} | ||
}) | ||
.then(execPipeline, execPipeline); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will swallow errors thrown from pMap
(as it doesn't work exactly as .finally(execPipeline))
.
Should it be .then(execPipeline, this.reject);
?
lib/pipeline.ts
Outdated
return execPipeline(); | ||
}); | ||
}) | ||
.then(execPipeline, execPipeline); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, should be .then(execPipeline, this.reject);
IMO
…cluster (#1467) Deliberately add `asCallback` on the **same** Promise instance being returned, not on a different Promise instance that resolves to the same thing. Avoid this error: ```javascript process.on('unhandledRejection', (reason) => console.log('unhandledRejection', reason)); const x = Promise.reject(new Error()); x.catch((e) => console.log('caught x', e)); const causesUnhandledRejection = Promise.resolve().then(() => x); ``` Related to #1466
529bed6
to
d4b8827
Compare
(When failing to load lua scripts or in Redis.Cluster edge cases) 1. Always return the same Promise in pipeline.exec. An example of why the original Promise should be returned, rather than a different Promise resolving to the original Promise: ```javascript process.on('unhandledRejection', (reason) => console.log('unhandledRejection', reason)); const x = Promise.reject(new Error()); x.catch((e) => console.log('caught x', e)); const causesUnhandledRejection = Promise.resolve().then(() => x); ``` 2. In the negligible chance the `script exists`/`script load` command failed, give up, catch the rejected Promise with `finally`, and try to run the commands in the pipeline anyway. (e.g. networking issues, corrupted bytes, etc) This isn't the best approach, but it's hopefully better than an unhandled Promise rejection, resource/memory leak, or hanging request due to a redis Promise never resolving or rejecting for an individual command in this edge case. 3. There are calls to `this.exec(...);` all over the Pipeline that don't check if the returned Promise is caught which I believe could lead to unhandled Promise rejections on retrying failed commands. Also, lib/autopipelining.js also called pipeline.exec without checking for errors. The fact there is now exactly one Promise instance that can be returned means that this no longer can cause unhandled Promise rejections. (see example snippet in commit description) (`standard-as-callback` is catching rejections) 4. pipeline.exec can explicitly call reject for ioredis's Redis.Cluster client
d4b8827
to
946efee
Compare
🎉 This PR is included in version 4.28.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [4.28.1](redis/ioredis@v4.28.0...v4.28.1) (2021-11-23) ### Bug Fixes * handle possible unhandled promise rejection with autopipelining+cluster ([#1467](redis/ioredis#1467)) ([6ad285a](redis/ioredis@6ad285a)), closes [#1466](redis/ioredis#1466)
## [4.28.2](redis/ioredis@v4.28.1...v4.28.2) (2021-12-01) ### Bug Fixes * add Redis campaign ([#1475](redis/ioredis#1475)) ([3f3d8e9](redis/ioredis@3f3d8e9)) * fix a memory leak with autopipelining. ([#1470](redis/ioredis#1470)) ([f5d8b73](redis/ioredis@f5d8b73)) * unhandled Promise rejections in pipeline.exec [skip ci] ([#1466](redis/ioredis#1466)) ([e5615da](redis/ioredis@e5615da))
Always return the same Promise in pipeline.exec.
An example of why the original Promise should be returned, rather than
a different Promise resolving to the original Promise:
In the negligible chance the
script exists
/script load
commandfailed, give up, catch the rejected Promise with
finally
,and try to run the commands in the pipeline anyway.
(e.g. networking issues, corrupted bytes, misbehaving proxy/server, etc)
This isn't the best approach, but it's hopefully better than an unhandled Promise rejection,
resource/memory leak, or hanging request due to a redis Promise never
resolving or rejecting for an individual command in this edge case.
If the maintainers have an idea for a better approach (e.g. rejecting requests in the pipeline with a single instance of a new error type) I'd be open to it
There are calls to
this.exec(...);
all over the Pipeline that don'tcheck if the returned Promise is caught which I believe could lead to
unhandled Promise rejections on retrying failed commands.
Also, lib/autopipelining.js also called pipeline.exec without
checking for errors.
The fact there is now exactly one Promise instance that can be returned
means that this no longer can cause unhandled Promise rejections.
(see example snippet in commit description)
(
standard-as-callback
is catching rejections)pipeline.exec can explicitly call reject for ioredis's Redis.Cluster
client
NOTE: This is theoretical. Due to the async nature of stack traces returning the stack trace of the place where the Error instance was created, the only stack traces I've seen are in redis-parser.
I've also seen unhandled rejections when loading scripts in an application using pipelining and Redis.Cluster, but I haven't figured out if this is the cause of the issues in that specific application