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 bug that scripts may not be loaded correctly in pipeline #1107

Merged
merged 1 commit into from Apr 21, 2020

Conversation

canyousayyes
Copy link
Contributor

@canyousayyes canyousayyes commented Apr 21, 2020

Hello, I want to report a bug in the pipeline.ts and submit the bug fix as well.


Description

In the script checking part:

return this.redis
  .script('exists', Array.from(new Set(scripts.map(({ sha }) => sha))))
  .then(function (results) {
    var pending = [];
    for (var i = 0; i < results.length; ++i) {
      if (!results[i]) {
        pending.push(scripts[i]);
      }
    }

It will pass a uniqued scripts array (Array.from(new Set(scripts.map(({ sha }) => sha)))) into SCRIPT EXISTS, this is fine.
However it will add the scripts from the original scripts array (pending.push(scripts[i])), this could introduce a problem.

The bug seems to be introduced since #781.


Example

If I defined 2 custom commands echo and test, and do:

redis.pipeline()
  .echo()
  .echo()
  .test()

Then the scripts will contain ["echo", "echo", "test"], but the uniqued scripts array is ["echo", "test"].
The inconsistencies between the two arrays will make echo script will be loaded twice but test script not loading into Redis.


Fix

Please review my PR for the fix. I have moved the uniqueness check when adding the scripts.
Now there's only one scripts array and there should be no more inconsistency issue.

I have also updated the unit test to cover this case.

@luin luin merged commit 072d460 into redis:master Apr 21, 2020
@luin
Copy link
Collaborator

luin commented Apr 21, 2020

Good catch! Thanks for your contribution!

ioredis-robot pushed a commit that referenced this pull request Apr 21, 2020
## [4.16.3](v4.16.2...v4.16.3) (2020-04-21)

### Bug Fixes

* scripts may not be loaded correctly in pipeline ([#1107](#1107)) ([072d460](072d460))
@ioredis-robot
Copy link
Collaborator

🎉 This PR is included in version 4.16.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@canyousayyes
Copy link
Contributor Author

Thanks for your swift response, I just need this new release for my app :)

janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this pull request Mar 1, 2024
## [4.16.3](redis/ioredis@v4.16.2...v4.16.3) (2020-04-21)

### Bug Fixes

* scripts may not be loaded correctly in pipeline ([#1107](redis/ioredis#1107)) ([072d460](redis/ioredis@072d460))
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.

None yet

3 participants