Fixes (#126)

* Fixes

* URL encode identity to ensure that characters like `&` are treated correctly
* Switch Dictionary to ConcurrentDictionary for cache so that it is thread-safe

* Fixes

* Remove diagnostic code

* Fixes

* Use UtcNow rather than Now to better handle daylight savings time transitions

* Fixes

* Use semaphore to ensure that we aren't flushing at the same time as data is being written (without this change data could be not flushed, or flushed multiple times)

* Fixes

* Remove diagnostic code

* Fixes

* Helpful comment

* Fixes

* Remove typo

* Fixes

* Placate the linter
* Change GET to POST if there are no traits
* Improve `payloads` variable name

* Fixes

* Reduce thread count to more reasonable number

Author: mjwills-k

Flagsmith.Client.Test/AnalyticsTests.cs

@@ -1,18 +1,58 @@
 using System;
-using Flagsmith;
+using System.Collections.Generic;
+using System.Diagnostics;
+using System.Linq;
 using System.Net.Http;
+using System.Threading;
 using System.Threading.Tasks;
 using Moq;
-using Xunit;
+using Newtonsoft.Json;
 using Newtonsoft.Json.Linq;
-using System.Threading;
-using System.Collections.Generic;
+using Xunit;
 
 namespace Flagsmith.FlagsmithClientTest
 {
     public class AnalyticsTests
     {
         const string _defaultApiUrl = "https://edge.api.flagsmith.com/api/v1/";
+
+        [Fact]
+        public async Task TestAnalyticsProcessorDoesNotThrowUnderLoad()
+        {
+            const int numberOfThreads = 500;
+            const int numberOfTracksPerThread = 7;
+            const string key = "TestAnalyticsProcessorFlushClearsAnalyticsDataFeature";
+
+            ThreadPool.SetMinThreads(numberOfThreads, numberOfThreads);
+            HttpMocker.PayloadsSubmitted = new System.Collections.Concurrent.ConcurrentBag<string>();
+
+            var mockHttpClient = HttpMocker.MockHttpResponse(System.Net.HttpStatusCode.OK, null, true);
+            var analyticsProcessor = new AnalyticsProcessorTest(mockHttpClient.Object, null, null);
+            var token = new CancellationToken();
+            await Parallel.ForEachAsync(Enumerable.Range(1, numberOfThreads), token, async (item, token) =>
+            {
+                for (int i = 0; i < numberOfTracksPerThread; i++)
+                {
+                    await analyticsProcessor.TrackFeature(key);
+                }
+                await analyticsProcessor.Flush();
+            });
+
+            var totalTrackedFeatureCount = HttpMocker.PayloadsSubmitted.Select(z =>
+            {
+                var data = JsonConvert.DeserializeObject<Dictionary<string, int>>(z);
+
+                return data.TryGetValue(key, out var count) ? count : 0;
+            }).Sum();
+
+            // Assert that the volume of entries flushed was as expected
+            Assert.Equal(numberOfTracksPerThread * numberOfThreads, totalTrackedFeatureCount);
+
+            await Task.Delay(11000);
+            // Make sure that Flush on track doesn't lock up
+            await analyticsProcessor.TrackFeature(key);
+        }
+
         [Fact]
         public void TestAnalyticsProcessorTrackFeatureUpdatesAnalyticsData()
         {
@@ -46,7 +86,7 @@ public async void TestAnalyticsProcessorFlushPostRequestDataMatchAnanlyticsData(
             await analyticsProcessor.TrackFeature("TestAnalyticsProcessorFlushPostRequestDataMatchAnanlyticsDataFeature2");
             var jObject = JObject.Parse(analyticsProcessor.ToString());
             await analyticsProcessor.Flush();
-            mockHttpClient.verifyHttpRequest(HttpMethod.Post, "/api/v1/analytics/flags/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Post, "/api/v1/analytics/flags/", Times.Once);
             Assert.Equal(1, jObject["TestAnalyticsProcessorFlushPostRequestDataMatchAnanlyticsDataFeature1"].Value<int>());
             Assert.Equal(1, jObject["TestAnalyticsProcessorFlushPostRequestDataMatchAnanlyticsDataFeature2"].Value<int>());
         }
@@ -71,7 +111,7 @@ public async Task TestAnalyticsProcessorCallingTrackFeatureCallsFlushWhenTimerRu
             var analyticsProcessor = new AnalyticsProcessorTest(mockHttpClient.Object, null, baseApiUrl: _defaultApiUrl);
             await Task.Delay(12 * 1000);
             await analyticsProcessor.TrackFeature("TestAnalyticsProcessorCallingTrackFeatureCallsFlushWhenTimerRunsOutFeature");
-            mockHttpClient.verifyHttpRequest(HttpMethod.Post, "/api/v1/analytics/flags/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Post, "/api/v1/analytics/flags/", Times.Once);
         }
 
         [Fact]

Flagsmith.Client.Test/FlagsmithTest.cs

@@ -1,14 +1,16 @@
-using Moq;
+using System;
 using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Net;
 using System.Net.Http;
+using System.Threading;
 using System.Threading.Tasks;
-using Xunit;
 using FlagsmithEngine.Environment.Models;
-using OfflineHandler;
+using Moq;
 using Newtonsoft.Json.Linq;
-using System.IO;
-using System;
-using System.Net;
+using OfflineHandler;
+using Xunit;
 
 namespace Flagsmith.FlagsmithClientTest
 {
@@ -23,7 +25,7 @@ public void TestFlagsmithStartsPollingManagerOnInitIfEnabled()
                 Content = new StringContent(Fixtures.JsonObject.ToString())
             });
             var flagsmithClientTest = new FlagsmithClient(Fixtures.ApiKey, httpClient: mockHttpClient.Object, enableClientSideEvaluation: true);
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
         }
         [Fact]
         public async void TestUpdateEnvironmentSetsEnvironment()
@@ -50,10 +52,10 @@ public async void TestUpdateEnvironmentSetsEnvironment()
             var flagsmithClientTest = new FlagsmithClient(Fixtures.ApiKey, enableClientSideEvaluation: true, httpClient: mockHttpClient.Object);
 
             // Then
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
             await flagsmithClientTest.GetEnvironmentFlags();
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/flags/", Times.Never);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Get, "/api/v1/flags/", Times.Never);
         }
         [Fact]
         public async Task TestGetEnvironmentFlagsCallsApiWhenNoLocalEnvironment()
@@ -65,7 +67,7 @@ public async Task TestGetEnvironmentFlagsCallsApiWhenNoLocalEnvironment()
             });
             var flagsmithClientTest = new FlagsmithClient(Fixtures.ApiKey, httpClient: mockHttpClient.Object);
             var flags = (await flagsmithClientTest.GetEnvironmentFlags()).AllFlags();
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/flags/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Get, "/api/v1/flags/", Times.Once);
             Assert.True(flags[0].Enabled);
             Assert.Equal("some-value", flags[0].Value);
             Assert.Equal("some_feature", flags[0].GetFeatureName());
@@ -95,18 +97,36 @@ public async Task TestGetEnvironmentFlagsUsesLocalEnvironmentWhenAvailable()
             var flagsmithClientTest = new FlagsmithClient(Fixtures.ApiKey, enableClientSideEvaluation: true, httpClient: mockHttpClient.Object);
 
             // Then
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
             var flags = (await flagsmithClientTest.GetEnvironmentFlags()).AllFlags();
             var fs = Fixtures.Environment.FeatureStates[0];
             Assert.Equal(fs.Enabled, flags[0].Enabled);
             Assert.Equal(fs.GetValue(), flags[0].Value);
             Assert.Equal(fs.Feature.Name, flags[0].GetFeatureName());
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
         }
         [Fact]
-        public async Task TestGetIdentityFlagsCallsGetApiWhenNoLocalEnvironmentNoTraits()
+        public async Task TestThatCacheDictionaryDoesNotThrowUnderLoad()
+        {
+            const int numberOfThreads = 500;
+
+            ThreadPool.SetMinThreads(numberOfThreads, numberOfThreads);
+
+            var mockHttpClient = HttpMocker.MockHttpResponse(System.Net.HttpStatusCode.OK, Fixtures.ApiIdentityResponse, false);
+
+            var flagsmithClientTest = new FlagsmithClient(Fixtures.ApiKey, httpClient: mockHttpClient.Object, cacheConfig: new CacheConfig(true));
+
+            var token = new CancellationToken();
+            await Parallel.ForEachAsync(Enumerable.Range(1, numberOfThreads), token, async (item, token) =>
+            {
+                var flags = (await flagsmithClientTest.GetIdentityFlags(item.ToString())).AllFlags();
+            });
+        }
+        [Theory]
+        [InlineData("identifier", "{\"identifier\":\"identifier\",\"traits\":[],\"transient\":false}")]
+        [InlineData("identifier&h=g", "{\"identifier\":\"identifier&h=g\",\"traits\":[],\"transient\":false}")]
+        public async Task TestGetIdentityFlagsCallsPostApiWhenNoLocalEnvironmentNoTraits(string identifier, string expectedJson)
         {
-            string identifier = "identifier";
             var mockHttpClient = HttpMocker.MockHttpResponse(new HttpResponseMessage
             {
                 StatusCode = System.Net.HttpStatusCode.OK,
@@ -117,8 +137,8 @@ public async Task TestGetIdentityFlagsCallsGetApiWhenNoLocalEnvironmentNoTraits(
             Assert.True(flags[0].Enabled);
             Assert.Equal("some-value", flags[0].Value);
             Assert.Equal("some_feature", flags[0].GetFeatureName());
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/identities/", Times.Once, new Dictionary<string, string> { { "identifier", identifier } });
 
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Post, "/api/v1/identities/", Times.Once, expectedJson);
         }
         [Fact]
         public async Task TestGetIdentityFlagsCallsPostApiWhenNoLocalEnvironmentWithTraits()
@@ -136,7 +156,7 @@ public async Task TestGetIdentityFlagsCallsPostApiWhenNoLocalEnvironmentWithTrai
             Assert.True(flags[0].Enabled);
             Assert.Equal("some-value", flags[0].Value);
             Assert.Equal("some_feature", flags[0].GetFeatureName());
-            mockHttpClient.verifyHttpRequest(HttpMethod.Post, "/api/v1/identities/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Post, "/api/v1/identities/", Times.Once);
 
         }
         [Fact]
@@ -151,11 +171,11 @@ public async Task TestGetIdentityFlagsUsesLocalEnvironmentWhenAvailable()
             var traits = new List<ITrait> { new Trait("foo", "bar") };
 
             var flagsmithClientTest = new FlagsmithClient(Fixtures.ApiKey, enableClientSideEvaluation: true, httpClient: mockHttpClient.Object);
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
 
             _ = await flagsmithClientTest.GetIdentityFlags("identifier", new List<ITrait>() { new Trait("foo", "bar") });
 
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
         }
         [Fact]
         public async Task TestGetIdentityFlagsUsesLocalIdentityOverridesWhenAvailable()
@@ -169,7 +189,7 @@ public async Task TestGetIdentityFlagsUsesLocalIdentityOverridesWhenAvailable()
             var traits = new List<ITrait> { new Trait("foo", "bar") };
 
             var flagsmithClientTest = new FlagsmithClient(Fixtures.ApiKey, enableClientSideEvaluation: true, httpClient: mockHttpClient.Object);
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Get, "/api/v1/environment-document/", Times.Once);
 
             var flags = await flagsmithClientTest.GetIdentityFlags("overridden-id", traits);
             var flag = await flags.GetFlag("some_feature");
@@ -282,7 +302,7 @@ public async Task TestGetIdentityFlagsSendsTraits()
 
             var flags = await flagsmithClient.GetIdentityFlags(identifier, traits);
 
-            mockHttpClient.verifyHttpRequest(HttpMethod.Post, "/api/v1/identities/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Post, "/api/v1/identities/", Times.Once);
             // TODO: verify the body is correct - I've verified manually but can't verify programmatically
         }
 
@@ -456,7 +476,7 @@ public async Task TestFlagsmithUsesTheAPIResponseEvenIfTheOfflineHandlerIsSet()
 
             // Then
             var environmentFlags = await flagsmithClientTest.GetEnvironmentFlags();
-            mockHttpClient.verifyHttpRequest(HttpMethod.Get, "/api/v1/flags/", Times.Once);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Get, "/api/v1/flags/", Times.Once);
             Assert.True(await environmentFlags.IsFeatureEnabled("some_feature"));
             Assert.NotEqual("offline-value", await environmentFlags.GetFeatureValue("some_feature"));
             Assert.Equal("some-value", await environmentFlags.GetFeatureValue("some_feature"));
@@ -521,13 +541,14 @@ public async Task TestAnalyticsDataConsistencyWithConcurrentCallsToGetFlags()
             });
             var flagsmithClientTest = new FlagsmithClient(Fixtures.ApiKey, httpClient: mockHttpClient.Object, enableAnalytics: true);
             var flags = await flagsmithClientTest.GetEnvironmentFlags();
-
-            Dictionary<string, int> featuresDictionary = new Dictionary<string, int>();
+            var featuresDictionary = new Dictionary<string, int>();
 
             const int numberOfFeatures = 10;
             const int numberOfThreads = 1000;
             const int callsPerThread = 1000;
 
+            ThreadPool.SetMinThreads(numberOfThreads, numberOfThreads);
+
             for (int i = 1; i <= numberOfFeatures; i++)
             {
                 featuresDictionary.TryAdd($"Feature_{i}", 0);
@@ -594,7 +615,7 @@ public async Task TestGetIdentityFlagsTransientIdentityCallsExpected()
             var flagsmithClient = new FlagsmithClient(Fixtures.ApiKey, httpClient: mockHttpClient.Object);
             var identityFlags = await flagsmithClient.GetIdentityFlags(identifier, traits, transient);
             // Then
-            mockHttpClient.verifyHttpRequest(HttpMethod.Post, "/api/v1/identities/", Times.Once, "{\"identifier\":\"transient_identity\",\"traits\":[{\"trait_key\":\"some_trait\",\"trait_value\":\"some_value\",\"transient\":false}],\"transient\":true}");
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Post, "/api/v1/identities/", Times.Once, "{\"identifier\":\"transient_identity\",\"traits\":[{\"trait_key\":\"some_trait\",\"trait_value\":\"some_value\",\"transient\":false}],\"transient\":true}");
             Assert.True(await identityFlags.IsFeatureEnabled("some_feature"));
             Assert.Equal("some-identity-trait-value", await identityFlags.GetFeatureValue("some_feature"));
         }
@@ -615,12 +636,12 @@ public async Task TestGetIdentityFlagsTransientTraitKeysCallsExpected()
             var flagsmithClient = new FlagsmithClient(Fixtures.ApiKey, httpClient: mockHttpClient.Object);
             var identityFlags = await flagsmithClient.GetIdentityFlags(identifier, traits);
             // Then
-            mockHttpClient.verifyHttpRequest(HttpMethod.Post, "/api/v1/identities/", Times.Once, "{\"identifier\":\"test_identity_with_transient_traits\",\"traits\":[{\"trait_key\":\"transient_trait\",\"trait_value\":\"transient_trait_value\",\"transient\":true}],\"transient\":false}");
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Post, "/api/v1/identities/", Times.Once, "{\"identifier\":\"test_identity_with_transient_traits\",\"traits\":[{\"trait_key\":\"transient_trait\",\"trait_value\":\"transient_trait_value\",\"transient\":true}],\"transient\":false}");
             Assert.True(await identityFlags.IsFeatureEnabled("some_feature"));
             Assert.Equal("some-transient-trait-value", await identityFlags.GetFeatureValue("some_feature"));
         }
         [Fact]
-        public async Task TestGetIdentityFlagsTransientIdentityWitoutTraitCallsExpected()
+        public async Task TestGetIdentityFlagsTransientIdentityWithoutTraitCallsExpected()
         {
             // Given
             string identifier = "transient_identity";
@@ -639,7 +660,7 @@ public async Task TestGetIdentityFlagsTransientIdentityWitoutTraitCallsExpected(
             var flagsmithClient = new FlagsmithClient(Fixtures.ApiKey, httpClient: mockHttpClient.Object);
             var identityFlags = await flagsmithClient.GetIdentityFlags(identifier, null, transient);
             // Then
-            mockHttpClient.verifyHttpRequestWithParams(HttpMethod.Get, "/api/v1/identities/", Times.Once, (Dictionary<string, string>)queryParams);
+            mockHttpClient.VerifyHttpRequest(HttpMethod.Post, "/api/v1/identities/", Times.Once, "{\"identifier\":\"transient_identity\",\"traits\":[],\"transient\":true}");
             Assert.True(await identityFlags.IsFeatureEnabled("some_feature"));
             Assert.Equal("some-identity-trait-value", await identityFlags.GetFeatureValue("some_feature"));
         }