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

Decline unsubscribe related command in non-subscribed mode #759

Merged
merged 4 commits into from
Aug 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/server.c
Original file line number Diff line number Diff line change
Expand Up @@ -4037,6 +4037,12 @@ int processCommand(client *c) {
return C_OK;
}

/* Not allow several UNSUBSCRIBE commands executed under non-pubsub mode */
if (!c->flag.pubsub && (c->cmd->proc == unsubscribeCommand || c->cmd->proc == sunsubscribeCommand ||
c->cmd->proc == punsubscribeCommand)) {
rejectCommandFormat(c, "-NOSUB '%s' command executed not in subscribed mode", c->cmd->fullname);
return C_OK;
}
/* Only allow commands with flag "t", such as INFO, REPLICAOF and so on,
* when replica-serve-stale-data is no and we are a replica with a broken
* link with primary. */
Expand Down
3 changes: 2 additions & 1 deletion tests/unit/info.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,8 @@ start_server {tags {"info" "external:skip"}} {
set info [r info clients]
assert_equal [getInfoProperty $info pubsub_clients] {1}
# non-pubsub clients should not be involved
assert_equal {0} [unsubscribe $rd2 {non-exist-chan}]
catch {unsubscribe $rd2 {non-exist-chan}} e
assert_match {*NOSUB*} $e
set info [r info clients]
assert_equal [getInfoProperty $info pubsub_clients] {1}
# close all clients
Expand Down
26 changes: 5 additions & 21 deletions tests/unit/pubsub.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,12 @@ start_server {tags {"pubsub network"}} {
$rd1 close
}

test "UNSUBSCRIBE from non-subscribed channels" {
test "UNSUBSCRIBE and PUNSUBSCRIBE from non-subscribed channels" {
set rd1 [valkey_deferring_client]
assert_equal {0 0 0} [unsubscribe $rd1 {foo bar quux}]

foreach command {unsubscribe punsubscribe} {
catch {$command $rd1 {foo bar quux}} e
assert_match {*NOSUB*} $e
}
# clean up clients
$rd1 close
}
Expand Down Expand Up @@ -202,14 +204,6 @@ start_server {tags {"pubsub network"}} {
$rd close
} {0} {resp3}

test "PUNSUBSCRIBE from non-subscribed channels" {
set rd1 [valkey_deferring_client]
assert_equal {0 0 0} [punsubscribe $rd1 {foo.* bar.* quux.*}]

# clean up clients
$rd1 close
}

test "NUMSUB returns numbers, not strings (#1561)" {
r pubsub numsub abc def
} {abc 0 def 0}
Expand Down Expand Up @@ -247,16 +241,6 @@ start_server {tags {"pubsub network"}} {
$rd1 close
}

test "PUNSUBSCRIBE and UNSUBSCRIBE should always reply" {
# Make sure we are not subscribed to any channel at all.
r punsubscribe
r unsubscribe
# Now check if the commands still reply correctly.
set reply1 [r punsubscribe]
set reply2 [r unsubscribe]
concat $reply1 $reply2
} {punsubscribe {} 0 unsubscribe {} 0}

### Keyspace events notification tests

test "Keyspace notifications: we receive keyspace notifications" {
Expand Down
7 changes: 3 additions & 4 deletions tests/unit/pubsubshard.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,8 @@ start_server {tags {"pubsubshard external:skip"}} {

test "SUNSUBSCRIBE from non-subscribed channels" {
set rd1 [valkey_deferring_client]
assert_equal {0} [sunsubscribe $rd1 {foo}]
assert_equal {0} [sunsubscribe $rd1 {bar}]
assert_equal {0} [sunsubscribe $rd1 {quux}]
catch {sunsubscribe $rd1 {foo}} e
assert_match {*NOSUB*} $e

# clean up clients
$rd1 close
Expand Down Expand Up @@ -169,4 +168,4 @@ start_server {tags {"pubsubshard external:skip"}} {
assert_equal {smessage chan1 world} [$rd1 read]
}
}
}
}
Loading