Skip to content
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

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

TysonAndre
Copy link
Collaborator

  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:

    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, 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

  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


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

TysonAndre added a commit to TysonAndre/ioredis that referenced this pull request Nov 23, 2021
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
@TysonAndre TysonAndre force-pushed the pipeline-exec-unhandled-rejection branch 2 times, most recently from e2892ad to 529bed6 Compare November 23, 2021 01:05
TysonAndre added a commit to TysonAndre/ioredis that referenced this pull request Nov 23, 2021
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
@luin luin self-requested a review November 23, 2021 07:46
lib/pipeline.ts Outdated
_this.redis._addedScriptHashes[scripts[i].sha] = true;
}
})
.then(execPipeline, execPipeline);
Copy link
Collaborator

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);
Copy link
Collaborator

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

luin pushed a commit that referenced this pull request Nov 23, 2021
…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
ioredis-robot pushed a commit that referenced this pull request Nov 23, 2021
## [4.28.1](v4.28.0...v4.28.1) (2021-11-23)

### Bug Fixes

* handle possible unhandled promise rejection with autopipelining+cluster ([#1467](#1467)) ([6ad285a](6ad285a)), closes [#1466](#1466)
@TysonAndre TysonAndre force-pushed the pipeline-exec-unhandled-rejection branch from 529bed6 to d4b8827 Compare November 23, 2021 15:06
(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
@TysonAndre TysonAndre force-pushed the pipeline-exec-unhandled-rejection branch from d4b8827 to 946efee Compare November 23, 2021 15:20
@luin luin changed the title Fix unhandled Promise rejections in pipeline.exec fix: unhandled Promise rejections in pipeline.exec [skip ci] Nov 24, 2021
@luin luin merged commit e5615da into redis:master Nov 24, 2021
ioredis-robot pushed a commit that referenced this pull request Dec 1, 2021
## [4.28.2](v4.28.1...v4.28.2) (2021-12-01)

### Bug Fixes

* add Redis campaign ([#1475](#1475)) ([3f3d8e9](3f3d8e9))
* fix a memory leak with autopipelining. ([#1470](#1470)) ([f5d8b73](f5d8b73))
* unhandled Promise rejections in pipeline.exec [skip ci] ([#1466](#1466)) ([e5615da](e5615da))
@ioredis-robot
Copy link
Collaborator

🎉 This PR is included in version 4.28.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this pull request Mar 1, 2024
## [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)
janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this pull request Mar 1, 2024
## [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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants