diff --git a/src/IO.Ably.Tests.Shared/Infrastructure/AblySandboxFixture.cs b/src/IO.Ably.Tests.Shared/Infrastructure/AblySandboxFixture.cs index 29076cbb9..bba429c99 100644 --- a/src/IO.Ably.Tests.Shared/Infrastructure/AblySandboxFixture.cs +++ b/src/IO.Ably.Tests.Shared/Infrastructure/AblySandboxFixture.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Net.Http; using System.Threading.Tasks; @@ -11,18 +12,48 @@ public class AblySandboxFixture { private static readonly DateTimeOffset StartInterval = DateHelper.CreateDate(DateTimeOffset.UtcNow.Year - 1, 2, 3, 15, 5); - private static readonly Dictionary Settings = new Dictionary(); + // One provisioning task per environment, created atomically. xunit.runner.json sets + // parallelizeTestCollections, and the sandbox is reached from several collections at once - + // ChannelSubscriptionsTests declares none at all, so it gets its own - which meant + // concurrent writes to a plain Dictionary. That can leave the indexer returning null for a + // key ContainsKey has just accepted, and the caller dereferences it: SandboxSpecs + // .GetRestClient throws NullReferenceException on the settings it was handed. The await + // between the check and the write also let two collections provision an app each. + // + // Lazy over a ConcurrentDictionary gives one Initialise per environment however many + // callers race, and hands every one of them the same task. + private static readonly ConcurrentDictionary>> Settings = + new ConcurrentDictionary>>(); public static async Task GetSettings(string environment = null) { environment = environment ?? "sandbox"; - if (Settings.ContainsKey(environment)) + + // Passed through, where it used to be dropped: Initialise defaults to "sandbox", so + // asking for any other environment provisioned a sandbox app and cached it under that + // environment's name. + var provisioning = Settings.GetOrAdd( + environment, + env => new Lazy>(() => Initialise(env))); + + try { - return Settings[environment]; + return await provisioning.Value; + } + catch + { + // A failure must not be what gets cached. The Dictionary this replaced assigned only + // on success, so a provisioning attempt that threw was retried by whoever asked + // next; holding the faulted task instead would fail every remaining sandbox test in + // the run with the same exception. + // + // Removed by key and value together, so a caller that has already raced in a + // replacement keeps it - ConcurrentDictionary's ICollection.Remove compares both, + // and the value comparison is reference equality on this exact Lazy. + ((ICollection>>>)Settings) + .Remove(new KeyValuePair>>(environment, provisioning)); + throw; } - - Settings[environment] = await Initialise(); - return Settings[environment]; } private static async Task Initialise(string environment = "sandbox")