Skip to content

fix cluster.sUnsubscribe - make listener optional #2946

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

Merged
merged 1 commit into from
May 7, 2025

Conversation

Clsan
Copy link
Contributor

@Clsan Clsan commented May 7, 2025

Description

  • I don't see any good reason for cluster.client.sUnsubscribe function must hasve listener.
  • So, I changed cluster.client.sUnsubscribe definition. (Make listener optional)
  • When sUnsubscribe is called, client.unsubscribe is called. #
  • It finally goes to pubSub.unsubscribe. #
  • If we register listener on sUnsubscribe, pubSubClient's isActive is true due to pre-registered listener. #
  • So, command is not actually issued. #
  • And, also not disconnected. #

Describe your pull request here


Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

@Clsan Clsan force-pushed the fix-sunsubscribe-def branch from 4107a37 to bd70de2 Compare May 7, 2025 06:39
@Clsan Clsan marked this pull request as draft May 7, 2025 06:43
@Clsan Clsan marked this pull request as ready for review May 7, 2025 06:59
@nkaradzhov nkaradzhov self-requested a review May 7, 2025 13:08
Copy link
Collaborator

@nkaradzhov nkaradzhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nkaradzhov nkaradzhov merged commit 86480aa into redis:master May 7, 2025
11 checks passed
@Clsan Clsan deleted the fix-sunsubscribe-def branch May 7, 2025 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants