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

unsubscribe failed when stringNumbers is True #1643

Closed
qishibo opened this issue Aug 30, 2022 · 5 comments · Fixed by #1710
Closed

unsubscribe failed when stringNumbers is True #1643

qishibo opened this issue Aug 30, 2022 · 5 comments · Fixed by #1710

Comments

@qishibo
Copy link

qishibo commented Aug 30, 2022

Hey luin, here is a bug on unsubscribe mode if set stringNumbers: true, code like this

const Redis = require("ioredis");

// const redis = new Redis({stringNumbers: false});
const redis = new Redis({stringNumbers: true});

redis.subscribe('xxx', (err, count) => {
  // unsubscribe
  redis.unsubscribe().then(r => {
    console.log('---unsub succ', r);
  }).catch(e => {
    console.log('===unsub fail', e);
  })
});

if stringNumbers is false, wverything goes well, '---unsub succ' will be logged, but if it is set to true, nothing will be logged and the promise will not be solved.

@qishibo
Copy link
Author

qishibo commented Aug 30, 2022

ioredis: 5.2.3
Redis: 5.0
Node: 16.14.2

@qishibo
Copy link
Author

qishibo commented Aug 30, 2022

if I pass param xxx in subscribe, the promise will be resolved, but in fact the redis instance is still in sub mode

const Redis = require("ioredis");

// const redis = new Redis({stringNumbers: false});
const redis = new Redis({stringNumbers: true});

redis.subscribe('xxx', (err, count) => {
  // unsubscribe
  redis.unsubscribe('xxx').then(r => {
    console.log('---unsub succ', r);

    // this will raise error "Connection in subscriber mode" too
    redis.get('name').then(r => {
      console.log('get name: ', r);
    });
  }).catch(e => {
    console.log('===unsub fail', e);
  })
});

@qishibo
Copy link
Author

qishibo commented Aug 31, 2022

line 157 locates the issue, the reply[2] is string instead of int when stringNumbers, so use == is better
https://github.com/luin/ioredis/blob/eabb70ed835828ad85b602a217992486e05d2218/lib/DataHandler.ts#L150-L160

@luin
Copy link
Collaborator

luin commented Jan 25, 2023

Ah good catch! Sorry I didn't notice the GitHub notification. Will take a look soon.

@luin luin added the bug label Jan 25, 2023
luin added a commit that referenced this issue Jan 25, 2023
luin added a commit that referenced this issue Jan 25, 2023
luin added a commit that referenced this issue Jan 25, 2023
luin added a commit that referenced this issue Jan 25, 2023
github-actions bot pushed a commit that referenced this issue Jan 25, 2023
# [5.3.0](v5.2.6...v5.3.0) (2023-01-25)

### Bug Fixes

* unsubscribe not work with stringNumbers ([#1710](#1710)) ([321f8de](321f8de)), closes [#1643](#1643)

### Features

* Add support ssubscribe ([#1690](#1690)) ([6285e80](6285e80))
@github-actions
Copy link

🎉 This issue has been resolved in version 5.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

luin added a commit that referenced this issue Apr 15, 2023
luin pushed a commit that referenced this issue Apr 15, 2023
# [5.3.0](v5.2.6...v5.3.0) (2023-01-25)

### Bug Fixes

* unsubscribe not work with stringNumbers ([#1710](#1710)) ([321f8de](321f8de)), closes [#1643](#1643)

### Features

* Add support ssubscribe ([#1690](#1690)) ([6285e80](6285e80))
janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this issue Mar 1, 2024
# [5.3.0](redis/ioredis@v5.2.6...v5.3.0) (2023-01-25)

### Bug Fixes

* unsubscribe not work with stringNumbers ([#1710](redis/ioredis#1710)) ([321f8de](redis/ioredis@321f8de)), closes [#1643](redis/ioredis#1643)

### Features

* Add support ssubscribe ([#1690](redis/ioredis#1690)) ([6285e80](redis/ioredis@6285e80))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants