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

Honor select disposition in transactions #2322

Merged
merged 4 commits into from
Dec 21, 2023
Merged

Honor select disposition in transactions #2322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5fef762
honor select disposition in transactions
slorello89 Dec 9, 2022
8226d63
rearranging if/switch
slorello89 Dec 9, 2022
f1c2931
using server instead of multiplxer
slorello89 Dec 9, 2022
87dc77c
release notes
slorello89 Dec 9, 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
2 changes: 1 addition & 1 deletion docs/ReleaseNotes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Current package versions:
- Fix [#2249](https://github.com/StackExchange/StackExchange.Redis/issues/2249): Properly handle a `fail` state (new `ClusterNode.IsFail` property) for `CLUSTER NODES` and expose `fail?` as a property (`IsPossiblyFail`) as well ([#2288 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2288))
- Adds: `IConnectionMultiplexer.ServerMaintenanceEvent` (was on `ConnectionMultiplexer` but not the interface) ([#2306 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2306))
- Adds: To timeout messages, additional debug information: `Sync-Ops` (synchronous operations), `Async-Ops` (asynchronous operations), and `Server-Connected-Seconds` (how long the connection in question has been connected, or `"n/a"`) ([#2300 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2300))

- Fix: [#2321](https://github.com/StackExchange/StackExchange.Redis/issues/2321): Honor disposition of select command in Command Map for transactions [(#2322 by slorello89)](https://github.com/StackExchange/StackExchange.Redis/pull/2322)

## 2.6.80

Expand Down
22 changes: 13 additions & 9 deletions src/StackExchange.Redis/RedisTransaction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,20 +116,24 @@ private void QueueMessage(Message message)
lock (SyncLock)
{
(_pending ??= new List<QueuedMessage>()).Add(queued);

switch (message.Command)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we move the test inside the switch? I'd rather not execute the IsAvailable check etc unless we think there's a good need; other than that: makes sense

Copy link
Collaborator

@mgravell mgravell Dec 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

not related to this change, but: unless I'm misreading the code, it looks like we'd issue this even on "cluster"; open question: should we also add a single-DB check? we usually know the DB count, either from server metadata (when available) or from the server type (note: DB count can be unknown)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good wrt to rearranging things.

WRT preventing an issue with cluster - what do you think of this new approach?

{
case RedisCommand.UNKNOWN:
case RedisCommand.EVAL:
case RedisCommand.EVALSHA:
// people can do very naughty things in an EVAL
// including change the DB; change it back to what we
// think it should be!
var sel = PhysicalConnection.GetSelectDatabaseCommand(message.Db);
queued = new QueuedMessage(sel);
wasQueued = SimpleResultBox<bool>.Create();
queued.SetSource(wasQueued, QueuedProcessor.Default);
_pending.Add(queued);
var server = multiplexer.SelectServer(message);
if (server != null && server.SupportsDatabases)
{
// people can do very naughty things in an EVAL
// including change the DB; change it back to what we
// think it should be!
var sel = PhysicalConnection.GetSelectDatabaseCommand(message.Db);
queued = new QueuedMessage(sel);
wasQueued = SimpleResultBox<bool>.Create();
queued.SetSource(wasQueued, QueuedProcessor.Default);
_pending.Add(queued);
}

break;
}
}
Expand Down
16 changes: 16 additions & 0 deletions tests/StackExchange.Redis.Tests/TransactionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1218,6 +1218,22 @@ public async Task CombineFireAndForgetAndRegularAsyncInTransaction()
Assert.Equal(30, count);
}

[Fact]
public async Task TransactionWithAdHocCommandsAndSelectDisabled()
{
using var conn = Create(disabledCommands: new string[] { "SELECT" });
RedisKey key = Me();
var db = conn.GetDatabase();
db.KeyDelete(key, CommandFlags.FireAndForget);
Assert.False(db.KeyExists(key));

var tran = db.CreateTransaction("state");
var a = tran.ExecuteAsync("SET", "foo", "bar");
Assert.True(await tran.ExecuteAsync());
var setting = db.StringGet("foo");
Assert.Equal("bar",setting);
}

#if VERBOSE
[Fact]
public async Task WatchAbort_StringEqual()
Expand Down