Fix #162 RedisScript fails for the CultureInfo tr-TR (and use BacklogPolicy.FailFast for backward compatibility) #173
Conversation
… BacklogPolicy.FailFast for backward compatibility)
| @@ -49,7 +49,7 @@ | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="StackExchange.Redis" Version="2.2.4" /> | |||
| <PackageReference Include="StackExchange.Redis" Version="2.6.122" /> | |||
There was a problem hiding this comment.
https://www.nuget.org/packages/StackExchange.Redis/2.6.122#versions-body-tab is the latest version as of today.
| @@ -35,7 +35,12 @@ private RedisServer(int? port, bool allowAdmin) | |||
| .RedirectStandardErrorTo(Console.Error); | |||
| ActiveServersByPort.Add(this.Port, this); | |||
| } | |||
| this.Multiplexer = ConnectionMultiplexer.Connect($"localhost:{this.Port}{(allowAdmin ? ",allowAdmin=true" : string.Empty)}"); | |||
| this.Multiplexer = ConnectionMultiplexer.Connect(new ConfigurationOptions | |||
There was a problem hiding this comment.
https://github.com/StackExchange/StackExchange.Redis/blob/5504ed9a93f729f204e6cb53280cb4efa1ccd390/docs/Configuration.md says that there are two ways to set configuration. I changed this to using ConfigurationOptions because BacklogPolicy is one of the code-only options.
| { | ||
| EndPoints = { { "localhost", this.Port } }, | ||
| AllowAdmin = allowAdmin, | ||
| BacklogPolicy = BacklogPolicy.FailFast, // for backward compatibility with pre-2.5 versions |
There was a problem hiding this comment.
Some tests started failing after upgrading to 2.5.27+ as noted here. I believe this is because of the following update (see the following excerpt from the release notes):
2.5.27 (prerelease)
- Adds: a backlog/retry mechanism for commands issued while a connection isn’t available (#1912 by NickCraver)
- Commands will be queued if a multiplexer isn’t yet connected to a Redis server.
- Commands will be queued if a connection is lost and then sent to the server when the connection is restored.
- All commands queued will only remain in the backlog for the duration of the configured timeout.
- To revert to previous behavior, a new ConfigurationOptions.BacklogPolicy is available - old behavior is configured via options.BacklogPolicy = BacklogPolicy.FailFast. This backlogs nothing and fails commands immediately if no connection is available.
There was a problem hiding this comment.
Interesting. Which tests fail without this? Based on the description I would expect it to be tests that reproduce things like connection failure.
My biggest concern is whether the need for this setting means that the library won't work for users who don't turn this on. Thoughts?
Finally, would be good to have a concise summary of the information in your comment + my questions above in the source itself.
There was a problem hiding this comment.
For (my) future reference, I just updated StackExchange.Redis to 2.5.43 and noticed the following two failures:
Core_ReaderWriterAsMutex_RedisReaderWriter_Redis2x1Database_RedisSynchronizationStrategy_Redis2x1Database_RedisSynchronizationStrategy_Redis2x1DatabaseTest
TestParallelism
Source: DistributedLockCoreTestCases.cs line 162
Duration: 30 sec
Message:
Timed out! (only 3/100 completed)
Expected: not same as <System.Threading.Tasks.Task+DelayPromise>
But was: <System.Threading.Tasks.Task+DelayPromise>
Core_Redis_Redis2x1Database_RedisSynchronizationStrategy_Redis2x1DatabaseTest
TestParallelism
Source: DistributedLockCoreTestCases.cs line 162
Duration: 30 sec
Message:
Timed out! (only 0/100 completed)
Expected: not same as <System.Threading.Tasks.Task+DelayPromise>
But was: <System.Threading.Tasks.Task+DelayPromise>
Note that the number of completed tests can vary.
There was a problem hiding this comment.
I ended up figuring out what I think is a good path forward here: https://github.com/madelson/DistributedLock/blob/release-2.4/src/DistributedLock.Redis/RedLock/RedLockAcquire.cs#L155
Appreciate you bringing this to my attention.
Bartleby2718
changed the title
| try | ||
| { | ||
| Thread.CurrentThread.CurrentCulture = new("tr-TR"); | ||
| CultureInfo.CurrentCulture.ClearCachedData(); |
There was a problem hiding this comment.
I would expect these 2 lines to just be:
CultureInfo.CurrentCulture = CultureInfo.GetCultureInfo("tr-TR");
| } | ||
| finally | ||
| { | ||
| Thread.CurrentThread.CurrentCulture = originalCultureInfo; |
There was a problem hiding this comment.
CultureInfo.CurrentCulture = originalCultureInfo;
|
|
||
| namespace Medallion.Threading.Tests.Redis; | ||
|
|
||
| [Category("CI")] | ||
| [NonParallelizable] // one of the tests temporarily changes CurrentCulture |
There was a problem hiding this comment.
We shouldn't need this. CurrentCulture is an asynclocal value (this is critical since otherwise you couldn't set different cultures for different concurrent web requests), so it shouldn't interfere with any concurrently-executing tests.
We only need to worry about this when setting truly-global values like environment variables that affect all threads at once.
|
@Bartleby2718 do you plan to follow up on this PR? |
|
@ I’d like to, but unfortunately, I don’t have a good answer to your “biggest concern” in #173 (comment). |
|
Closing since I ended up reworking the code a quite a bit to solve for the backlog/failfast issue discussed above. The test you wrote has been integrated. Thanks @Bartleby2718 ! |
Hi @madelson, here's my first PR in this repo! Looking forward to your feedback on this PR.