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

Legacy command no longer call callback with error #2392

Closed
msebire opened this issue Jan 27, 2023 · 1 comment · Fixed by #2394
Closed

Legacy command no longer call callback with error #2392

msebire opened this issue Jan 27, 2023 · 1 comment · Fixed by #2394
Labels

Comments

@msebire
Copy link

msebire commented Jan 27, 2023

Description

Since @redis/client 1.5.0, callback are no longer called when an error is thrown.

Issue can be reproduced using the following code:

  const client = createClient({ url, legacyMode: true });
  
  await client.connect();

  const result = await promisify(client.sendCommand).call(client, 'PING');
  console.log(`Result is ${result}`);

> OK "Result is PONG" is printed

  try {
    await promisify(client.sendCommand).call(client, 'FAIL');
  } catch (e) {
    console.log(`Error is ${e}`);
> KO, we never reach this code
  }

From what I was able to investigate, it seems the issue was introduced here: aa75ee49#diff-6674346081a0261b88e15fb0ae5a540ec522569c7310ff00a34b4b0c34a0c07aL316
as the catch mechanism to forward the error to the callback was removed.

To give you a bit more context on my use case, I encountered this issue because I use redlock as dependency of my project.
Redlock use the callback error mechanism to load scripts if they are missing.
https://github.com/mike-marcacci/node-redlock/blob/870778cd520f9649e80f74fa755ee0f12519c683/redlock.js#L455

Node.js Version

v19.4.0

Redis Server Version

6.2.10

Node Redis Version

4.6.1

Platform

windows

Logs

No response

@leibale
Copy link
Collaborator

leibale commented Jan 27, 2023

@msebire version 4.6.2 is on npm, sorry about that

edit: BTW, I've added tests so it won't happen again..

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 a pull request may close this issue.

2 participants