Sometimes execResult is undefined. #858

rimiti · 2019-05-02T19:03:47Z

Hello,

Sometimes, execResult return is undefined. In this case, we just have to return.

Note: Commit updated.

Regards,

rimiti · 2019-05-03T16:24:01Z

@luin Could you please merge this PR? 🙏

luin · 2019-05-03T17:03:39Z

@rimiti Thank you for the pull request. I'm wondering in which case the pipeline will return void?

rimiti · 2019-05-03T19:03:19Z

It occurred when I had a transaction with some del actions. I can’t share you a snipped code because it’s from my client software and the logic is too complicated to extract you an example.

Otherwise the fix doesn’t break the CI. It cant be safely merged.

Note: if you’re looking for a maintainer, I can help you.

luin · 2019-05-04T02:38:58Z

I just did some tests on my side, but the results seem to always be arrays no matter what the commands were in the transaction. Here's a sample:

redis.multi().del('foo1').del('foo2').exec(console.log)

The output is:

null [ [ null, 1 ], [ null, 1 ] ]

Could you create a minimum reproducible sample for this fix?

rimiti · 2019-05-04T10:47:21Z

I've re-run the tests with adding a console.log() when execResult is undefined:

if (typeof execResult === 'undefined') {         
    console.log(pipeline);
    return;
}

Output:

tests    |        ...
tests    |        [ Command {
tests    |            name: 'multi',
tests    |            transformed: true,
tests    |            isCustomCommand: false,
tests    |            replyEncoding: 'utf8',
tests    |            errorStack: undefined,
tests    |            args: [],
tests    |            callback: undefined,
tests    |            resolve: [Function],
tests    |            reject: [Function],
tests    |            promise: [Promise] },
tests    |          Command {
tests    |            name: 'srem',
tests    |            transformed: true,
tests    |            isCustomCommand: false,
tests    |            replyEncoding: 'utf8',
tests    |            errorStack: undefined,
tests    |            args: [Array],
tests    |            callback: undefined,
tests    |            resolve: [Function],
tests    |            reject: [Function],
tests    |            promise: [Promise] },
tests    |          Command {
tests    |            name: 'srem',
tests    |            transformed: true,
tests    |            isCustomCommand: false,
tests    |            replyEncoding: 'utf8',
tests    |            errorStack: undefined,
tests    |            args: [Array],
tests    |            callback: undefined,
tests    |            resolve: [Function],
tests    |            reject: [Function],
tests    |            promise: [Promise] },
tests    |          Command {
tests    |            name: 'del',
tests    |            transformed: true,
tests    |            isCustomCommand: false,
tests    |            replyEncoding: 'utf8',
tests    |            errorStack: undefined,
tests    |            args: [Array],
tests    |            callback: undefined,
tests    |            resolve: [Function],
tests    |            reject: [Function],
tests    |            promise: [Promise] },
tests    |          Command {
tests    |            name: 'del',
tests    |            transformed: true,
tests    |            isCustomCommand: false,
tests    |            replyEncoding: 'utf8',
tests    |            errorStack: undefined,
tests    |            args: [Array],
tests    |            callback: undefined,
tests    |            resolve: [Function],
tests    |            reject: [Function],
tests    |            promise: [Promise] },
tests    |          Command {
tests    |            name: 'srem',
tests    |            transformed: true,
tests    |            isCustomCommand: false,
tests    |            replyEncoding: 'utf8',
tests    |            errorStack: undefined,
tests    |            args: [Array],
tests    |            callback: undefined,
tests    |            resolve: [Function],
tests    |            reject: [Function],
tests    |            promise: [Promise] },
tests    |          Command {
tests    |            name: 'srem',
tests    |            transformed: true,
tests    |            isCustomCommand: false,
tests    |            replyEncoding: 'utf8',
tests    |            errorStack: undefined,
tests    |            args: [Array],
tests    |            callback: undefined,
tests    |            resolve: [Function],
tests    |            reject: [Function],
tests    |            promise: [Promise] },
tests    |          Command {
tests    |            name: 'exec',
tests    |            transformed: true,
tests    |            isCustomCommand: false,
tests    |            replyEncoding: null,
tests    |            errorStack: undefined,
tests    |            args: [],
tests    |            callback: undefined,
tests    |            resolve: [Function],
tests    |            reject: [Function],
tests    |            promise: [Promise] },
tests    |          Command {
tests    |            name: 'del',
tests    |            transformed: true,
tests    |            isCustomCommand: false,
tests    |            replyEncoding: 'utf8',
tests    |            errorStack: undefined,
tests    |            args: [Array],
tests    |            callback: undefined,
tests    |            resolve: [Function],
tests    |            reject: [Function],
tests    |            promise: [Promise] },
tests    |          Command {
tests    |            name: 'del',
tests    |            transformed: true,
tests    |            isCustomCommand: false,
tests    |            replyEncoding: 'utf8',
tests    |            errorStack: undefined,
tests    |            args: [Array],
tests    |            callback: undefined,
tests    |            resolve: [Function],
tests    |            reject: [Function],
tests    |            promise: [Promise] } ],
tests    |       _result:
tests    |        [ [ null, 'OK' ],
tests    |          [ null, 'QUEUED' ],
tests    |          [ null, 'QUEUED' ],
tests    |          [ null, 'QUEUED' ],
tests    |          [ null, 'QUEUED' ],
tests    |          [ null, 'QUEUED' ],
tests    |          [ null, 'QUEUED' ],
tests    |          [ null, [Array] ],
tests    |          undefined,
tests    |          undefined ],
tests    |       _transactions: 0,
tests    |       _shaToScript: {},
tests    |       resolve: [Function],
tests    |       reject: [Function],
tests    |       promise:
tests    |        Promise {
tests    |          [ [Array],
tests    |            [Array],
tests    |            [Array],
tests    |            [Array],
tests    |            [Array],
tests    |            [Array],
tests    |            [Array],
tests    |            [Array],
tests    |            undefined,
tests    |            undefined ] },
tests    |       exec: [Function],
tests    |       execBuffer: [Function],
tests    |       nodeifiedPromise: true,
tests    |       replyPending: 0 }".
tests    |
tests    |       at BufferedConsole.log (node_modules/@jest/console/build/BufferedConsole.js:199:10)
tests    |       at node_modules/ioredis/built/transaction.js:47:25

I hope it can help you.

luin · 2019-05-04T12:54:05Z

Thanks for the logs. I can finally reproduce the issue. The key point is to send other commands after exec():

const pipeline = redis.multi().del('foo1')
pipeline.exec(console.log) // TypeError: Cannot read property '0' of undefined
pipeline.del('foo3') // pipeline shouldn't be used to send any commands when the `exec()` has been called on it.

I think we should catch the exception (like this pr do) but re-throw it as a more meaningful error with a message like "Transaction resolved failed. Did you call any commands in a transaction after exec() had been called on that transaction?"

What do you think?

rimiti · 2019-05-04T13:50:40Z

I think we should catch the exception (like this pr do) but re-throw it as a more meaningful error with a message like "Transaction resolved failed. Did you call any commands in a transaction after exec() had been called on that transaction?"

I think there are two solutions:

First: Throwing an error when we are in this case.
Second: Adding commands sent after exec() into the pipeline array.

I prefer the second solution because, it always works.

luin · 2019-05-04T14:10:02Z

First: Throwing an error when we are in this case.

This is the quickest solution for this issue.

Second: Adding commands sent after exec() into the pipeline array.

This breaks BC since in the past commands invoked after exec() will not be sent to Redis. So even we chose this way, it have to be happened in the next major release.

Another cons I can think of is allowing commands after exec() makes things obscure: Will these command be included in the transaction, or be excluded from the transaction?

rimiti · 2019-05-04T14:57:03Z

Another cons I can think of is allowing commands after exec() makes things obscure: Will these command be included in the transaction, or be excluded from the transaction?

I think, if it's obscure, it's because this is not the right way to do that. After reflection, it'll be more clear to just throw an error instead of trying to bypass transaction logic.

What disturbs me, if I run the same tests with redis package, it's works and without error.
It may be interesting to check how to the redis package treat this case ?

luin · 2019-05-04T15:13:45Z

Just tried node_redis. The following code：

const m = r.multi().del('foo1')
m.exec(console.log)
m.del('foo2')

Both node_redis & ioredis will simply ignore del(foo2). node_redis returns [0, undefined] corresponding to del(foo1) and del(foo2). The result of del(foo2) being undefined is because the command was not sent to Redis.

I think in this case, simply throwing errors to let developers know that there are commands not being sent would be a fair solution.

rimiti · 2019-05-04T15:37:16Z

Both node_redis & ioredis will simply ignore del(foo2). node_redis returns [0, undefined] corresponding to del(foo1) and del(foo2). The result of del(foo2) being undefined is because the command was not sent to Redis.

Oh, it's so a silent error... 😕
Totally agreed with you, throwing an error is best solution.

rimiti · 2019-05-06T11:18:12Z

@luin Do you want I throw an error like that instead of the return?

Before

        if (typeof execResult === 'undefined') {
          return
        }

After

        if (typeof execResult === 'undefined') {
          throw new Error('Pipeline cannot be used to send any commands when the `exec()` has been called on it.');
        }

What do you think?

luin · 2019-05-06T14:21:34Z

Looks good to me!

…called

rimiti · 2019-05-07T07:41:31Z

It can be merged. 🎉

## [4.9.3](v4.9.2...v4.9.3) (2019-05-07) ### Bug Fixes * more meaningful errors when using pipeline after exec(). ([#858](#858)) ([0c3ef01](0c3ef01))

rimiti · 2019-05-07T14:31:34Z

@luin Could you please publish on npm this new version?

luin · 2019-05-07T14:52:44Z

@rimiti The new version has been published automatically when merged. I just add the 4.9.3 to the latest tag.

## [4.9.3](redis/ioredis@v4.9.2...v4.9.3) (2019-05-07) ### Bug Fixes * more meaningful errors when using pipeline after exec(). ([#858](redis/ioredis#858)) ([0c3ef01](redis/ioredis@0c3ef01))

rimiti changed the title ~~WIP - Sometimes promise return nothing~~ Sometimes promise return void (undefined) May 2, 2019

fix(lib/transaction.ts) sometimes execResult is undefined

50b9ec5

rimiti force-pushed the fix-undefined-results branch from ed25cdb to 50b9ec5 Compare May 4, 2019 10:37

rimiti changed the title ~~Sometimes promise return void (undefined)~~ Sometimes execResult is undefined. May 4, 2019

fix(lib/transaction.ts) throw an error when .exec() has already been …

733f239

…called

luin merged commit 0c3ef01 into redis:master May 7, 2019

ioredis-robot pushed a commit that referenced this pull request May 7, 2019

chore(release): 4.9.3 [skip ci]

519d481

## [4.9.3](v4.9.2...v4.9.3) (2019-05-07) ### Bug Fixes * more meaningful errors when using pipeline after exec(). ([#858](#858)) ([0c3ef01](0c3ef01))

rimiti deleted the fix-undefined-results branch May 7, 2019 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sometimes execResult is undefined. #858

Sometimes execResult is undefined. #858

rimiti commented May 2, 2019 •

edited

Loading

rimiti commented May 3, 2019

luin commented May 3, 2019

rimiti commented May 3, 2019

luin commented May 4, 2019

rimiti commented May 4, 2019

luin commented May 4, 2019

rimiti commented May 4, 2019 •

edited

Loading

luin commented May 4, 2019 •

edited

Loading

rimiti commented May 4, 2019

luin commented May 4, 2019

rimiti commented May 4, 2019

rimiti commented May 6, 2019

luin commented May 6, 2019

rimiti commented May 7, 2019

rimiti commented May 7, 2019

luin commented May 7, 2019

Sometimes execResult is undefined. #858

Sometimes execResult is undefined. #858

Conversation

rimiti commented May 2, 2019 • edited Loading

rimiti commented May 3, 2019

luin commented May 3, 2019

rimiti commented May 3, 2019

luin commented May 4, 2019

rimiti commented May 4, 2019

luin commented May 4, 2019

rimiti commented May 4, 2019 • edited Loading

luin commented May 4, 2019 • edited Loading

rimiti commented May 4, 2019

luin commented May 4, 2019

rimiti commented May 4, 2019

rimiti commented May 6, 2019

Before

After

luin commented May 6, 2019

rimiti commented May 7, 2019

rimiti commented May 7, 2019

luin commented May 7, 2019

rimiti commented May 2, 2019 •

edited

Loading

rimiti commented May 4, 2019 •

edited

Loading

luin commented May 4, 2019 •

edited

Loading