Move TCL test unit/pubsub to Go case #982
Conversation
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
| }, receiveType(t, pubsub, &redis.Subscription{})) | ||
|
|
||
| require.EqualValues(t, 0, rdb.Publish(ctx, "chan1", "hello").Val()) | ||
| require.EqualValues(t, 0, rdb.Publish(ctx, "chan2", "hello").Val()) |
There was a problem hiding this comment.
--- FAIL: TestPubSub (1.08s)
--- FAIL: TestPubSub/PUBLISH/SUBSCRIBE_basics (0.00s)
pubsub_test.go:76:
Error Trace: /home/runner/work/incubator-kvrocks/incubator-kvrocks/tests/gocase/unit/pubsub/pubsub_test.go:76
Error: Not equal:
expected: int(0)
actual : int64(1)
Test: TestPubSub/PUBLISH/SUBSCRIBE_basics
--- FAIL: TestPubSub/PUBLISH/SUBSCRIBE_after_UNSUBSCRIBE_without_arguments (0.00s)
pubsub_test.go:129:
Error Trace: /home/runner/work/incubator-kvrocks/incubator-kvrocks/tests/gocase/unit/pubsub/pubsub_test.go:129
Error: Not equal:
expected: int(0)
actual : int64(1)
Test: TestPubSub/PUBLISH/SUBSCRIBE_after_UNSUBSCRIBE_without_arguments
@vmihailenco do you have ideas on this failure? Why can we still publish to a channel even if no one is subscribing to it? It seems like an unstable case instead of a steady one.
There was a problem hiding this comment.
| require.EqualValues(t, 2, receiveType(t, pubsub, &redis.Subscription{}).Count) | ||
| require.EqualValues(t, 3, receiveType(t, pubsub, &redis.Subscription{}).Count) | ||
|
|
||
| require.NoError(t, pubsub.Unsubscribe(ctx)) |
There was a problem hiding this comment.
@tisonkun looks like you need to add channels here - "chan1", "chan2", "chan3". Not sure why the test is not failing earlier though and you still receive unsubscribe events... Perhaps some magic by kvrocks when the list of channels is empty? I wonder if Redis behaves the same.
There was a problem hiding this comment.
@vmihailenco from Redis command docs:
When no channels are specified, the client is unsubscribed from all the previously subscribed channels. In this case, a message for every unsubscribed channel will be sent to the client.
... from go-redis method docs:
// Unsubscribe the client from the given channels, or from all of
// them if none is given.
func (c *PubSub) Unsubscribe(ctx context.Context, channels ...string) error {So I think this command should behave as "unsubscribe all" otherwise it may be a bug.
The interesting point is that, even we check the unsubscription event happened, we can still publish to those channels. @git-hulk this can be a multi-thread concurrency issue for Kvrocks specifically - I'm unsure about it.
There was a problem hiding this comment.
from go-redis method docs:
Indeed! I honestly can't find where that is implemented and can't check it right away so don't trust it blindly :)
Perhaps it indeed unsubscribes thanks to Redis/kvrocks behavior, but does not reset the internal state of PubSub which can be a cause for surprises.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Forgive my comment above, it's about stateful tests.
There was a problem hiding this comment.
You can add defer pubsub.Close() to the tests to properly cleanup pubsub and go-redis should fix redis/go-redis#2248
There was a problem hiding this comment.
So now I get the point:
Because we don't wait for unsubscription in L76, the "chan2" subscription will never be unsubscribed and leak.
There was a problem hiding this comment.
We may try to fail fast or reduce share state...But pubsub state are on the server side so the only approach is start a new server for each case?
There was a problem hiding this comment.
@vmihailenco thank you! Let me think of it.
This closes #889.