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.

Sign up for GitHub

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

Jump to bottom

for pub/sub, treat length-one arrays comparably to simple values #2118

Merged
merged 3 commits into from
Apr 26, 2022
Merged

for pub/sub, treat length-one arrays comparably to simple values #2118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6bd27e9
fix #2117
mgravell Apr 26, 2022
c855770
update release notes
mgravell Apr 26, 2022
a8183a2
actually check whether we're meant to be allowing arrays
mgravell Apr 26, 2022
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
1 change: 1 addition & 0 deletions docs/ReleaseNotes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
- Adds: Support for `OBJECT FREQ` with `.KeyFrequency()`/`.KeyFrequencyAsync()` ([#2105 by Avital-Fine](https://github.com/StackExchange/StackExchange.Redis/pull/2105))
- Performance: Avoids allocations when computing cluster hash slots or testing key equality ([#2110 by Marc Gravell](https://github.com/StackExchange/StackExchange.Redis/pull/2110))
- Adds: Support for `SORT_RO` with `.Sort()`/`.SortAsync()` ([#2111 by slorello89](https://github.com/StackExchange/StackExchange.Redis/pull/2111))
- Adds: Support for pub/sub payloads that are unary arrays ([#2118 by Marc Gravell](https://github.com/StackExchange/StackExchange.Redis/pull/2118))

## 2.5.61

Expand Down
29 changes: 25 additions & 4 deletions src/StackExchange.Redis/PhysicalConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1498,10 +1498,10 @@ private void MatchResult(in RawResult result)
// invoke the handlers
var channel = items[1].AsRedisChannel(ChannelPrefix, RedisChannel.PatternMode.Literal);
Trace("MESSAGE: " + channel);
if (!channel.IsNull)
if (!channel.IsNull && TryGetPubSubPayload(items[2], out var payload))
{
_readStatus = ReadStatus.InvokePubSub;
muxer.OnMessage(channel, channel, items[2].AsRedisValue());
muxer.OnMessage(channel, channel, payload);
}
return; // AND STOP PROCESSING!
}
Expand All @@ -1511,11 +1511,11 @@ private void MatchResult(in RawResult result)

var channel = items[2].AsRedisChannel(ChannelPrefix, RedisChannel.PatternMode.Literal);
Trace("PMESSAGE: " + channel);
if (!channel.IsNull)
if (!channel.IsNull && TryGetPubSubPayload(items[3], out var payload))
{
var sub = items[1].AsRedisChannel(ChannelPrefix, RedisChannel.PatternMode.Pattern);
_readStatus = ReadStatus.InvokePubSub;
muxer.OnMessage(sub, channel, items[3].AsRedisValue());
muxer.OnMessage(sub, channel, payload);
}
return; // AND STOP PROCESSING!
}
Expand Down Expand Up @@ -1551,6 +1551,27 @@ private void MatchResult(in RawResult result)
}
_readStatus = ReadStatus.MatchResultComplete;
_activeMessage = null;

static bool TryGetPubSubPayload(in RawResult value, out RedisValue parsed, bool allowArraySingleton = true)
{
if (value.IsNull)
{
parsed = RedisValue.Null;
return true;
}
switch (value.Type)
{
case ResultType.Integer:
case ResultType.SimpleString:
case ResultType.BulkString:
parsed = value.AsRedisValue();
return true;
case ResultType.MultiBulk when allowArraySingleton && value.ItemsCount == 1:
return TryGetPubSubPayload(in value[0], out parsed, allowArraySingleton: false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW I think this would be simpler to take the first element as a RedisValue instead of recursion just for ease of understanding/maintenance, but there may be a future case of nested arrays I'm unaware of.

Copy link
Collaborator Author

@mgravell mgravell Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@NickCraver yah, avoiding edge cases and avoiding having to repeat the "what combinations are allowed" (the switch); ultimately the array here was unexpected, so... yeah: I don't want to trust anything

}
parsed = default;
return false;
}
}

private volatile Message? _activeMessage;
Expand Down