diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cd60746..e0f5c6d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,8 @@ ## XX.XX.XX +* Fixed public APIs (RecordEvent, RecordView, RecordException, SetLocation, DisableLocation, StartEvent, EndEvent, CancelEvent, AddCrashBreadCrumb, ChangeDeviceId, SetConsent) throwing a NullReferenceException when called before "Init"; they now safely no-op until the SDK is initialized. +* Fixed the SDK constructing a new HttpClient for every request, which could exhaust sockets under sustained use; a single shared client with a request timeout is now reused. +* Fixed the .NET Framework (net35/net45) networking path leaking the HTTP response stream and losing the real status code on 4xx/5xx responses. +* Fixed unique-timestamp generation not being thread-safe, which could hand out duplicate timestamps when recording from multiple threads at once. * Added support for a Log Listener: a callback set at initialization via the new configuration option "SetLogListener" that receives every SDK log message and its level. It fires independently of the console logging flag, so SDK logs can be captured in release builds without printing to the console. * Added support for SDK Health Checks: once per initialization, right after the SDK Behavior Settings fetch, the SDK sends a non-queued direct request to "/i" reporting internal warning/error log counts and the last failed request's status/body. This can be turned off with the new configuration option "DisableHealthCheck()". * Added support for SDK Behavior Settings (Server Config), enabled by default: diff --git a/countlyCommon/TestingRelated/PreInitSafetyTests.cs b/countlyCommon/TestingRelated/PreInitSafetyTests.cs new file mode 100644 index 00000000..c8090f32 --- /dev/null +++ b/countlyCommon/TestingRelated/PreInitSafetyTests.cs @@ -0,0 +1,92 @@ +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using CountlySDK; +using Xunit; +using static CountlySDK.CountlyCommon.CountlyBase; + +namespace TestProject_common +{ + /// + /// Public entry points must not crash the host app when they are called before Init + /// (a common race: an async Init running while telemetry calls arrive). In the + /// never-initialized state Configuration is null, and several guards used to dereference + /// Configuration.backendMode before the IsInitialized() check, throwing a + /// NullReferenceException. These tests pin the "no throw, no-op" contract. + /// + public class PreInitSafetyTests : IDisposable + { + public PreInitSafetyTests() + { + TestHelper.CleanDataFiles(); + Countly.Halt(); + TestHelper.CleanDataFiles(); + // Simulate the genuine never-initialized state: Halt clears ServerUrl/AppKey but + // intentionally leaves Configuration set, so we null it explicitly here. + Countly.Instance.Configuration = null; + } + + public void Dispose() + { + } + + [Fact] + public async Task RecordEvent_BeforeInit_ReturnsFalseWithoutThrowing() + { + bool result = await Countly.RecordEvent("pre_init_event"); + Assert.False(result); + } + + [Fact] + public async Task RecordView_BeforeInit_ReturnsFalseWithoutThrowing() + { + bool result = await Countly.Instance.RecordView("pre_init_view"); + Assert.False(result); + } + + [Fact] + public async Task RecordException_BeforeInit_ReturnsFalseWithoutThrowing() + { + bool result = await Countly.RecordException("err", "stack", null, false); + Assert.False(result); + } + + [Fact] + public async Task SetLocation_BeforeInit_ReturnsFalseWithoutThrowing() + { + bool result = await Countly.Instance.SetLocation("12.34,56.78"); + Assert.False(result); + } + + [Fact] + public async Task DisableLocation_BeforeInit_ReturnsFalseWithoutThrowing() + { + bool result = await Countly.Instance.DisableLocation(); + Assert.False(result); + } + + [Fact] + public void StartEvent_BeforeInit_DoesNotThrow() + { + Countly.Instance.StartEvent("pre_init_timed"); + } + + [Fact] + public void AddCrashBreadCrumb_BeforeInit_DoesNotThrow() + { + Countly.Instance.AddCrashBreadCrumb("crumb"); + } + + [Fact] + public async Task ChangeDeviceId_BeforeInit_DoesNotThrow() + { + await Countly.Instance.ChangeDeviceId("new_device_id"); + } + + [Fact] + public async Task SetConsent_BeforeInit_DoesNotThrow() + { + await Countly.Instance.SetConsent(new Dictionary { { ConsentFeatures.Events, true } }); + } + } +} diff --git a/countlyCommon/TestingRelated/TimeHelperTests.cs b/countlyCommon/TestingRelated/TimeHelperTests.cs new file mode 100644 index 00000000..269397d8 --- /dev/null +++ b/countlyCommon/TestingRelated/TimeHelperTests.cs @@ -0,0 +1,46 @@ +using System.Collections.Concurrent; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using CountlySDK.Helpers; +using Xunit; + +namespace TestProject_common +{ + public class TimeHelperTests + { + /// + /// GetUniqueUnixTime must hand out a strictly-unique value on every call, even when + /// hammered from many threads at once. The counter is a read-modify-write on a shared + /// field, so without synchronization two threads can observe the same value and both + /// return it. This drives concurrent callers hard and asserts every value is unique. + /// + [Fact] + public void GetUniqueUnixTime_UnderConcurrency_ReturnsAllUniqueValues() + { + TimeHelper timeHelper = new TimeHelper(); + const int threadCount = 8; + const int perThread = 20000; + + ConcurrentQueue results = new ConcurrentQueue(); + + using (Barrier barrier = new Barrier(threadCount)) { + Task[] tasks = new Task[threadCount]; + for (int t = 0; t < threadCount; t++) { + tasks[t] = Task.Run(() => { + barrier.SignalAndWait(); + for (int i = 0; i < perThread; i++) { + results.Enqueue(timeHelper.GetUniqueUnixTime()); + } + }); + } + + Task.WaitAll(tasks); + } + + int total = threadCount * perThread; + int distinct = results.Distinct().Count(); + Assert.Equal(total, distinct); + } + } +} diff --git a/countlyCommon/countlyCommon/CountlyBase.cs b/countlyCommon/countlyCommon/CountlyBase.cs index 894a49ba..12b68795 100644 --- a/countlyCommon/countlyCommon/CountlyBase.cs +++ b/countlyCommon/countlyCommon/CountlyBase.cs @@ -527,7 +527,7 @@ private async Task UploadSessions() /// public void StartEvent(string key) { - if (Configuration.backendMode) { + if (Configuration?.backendMode ?? false) { UtilityHelper.CountlyLogging("[CountlyBase] StartEvent, Backend Mode enabled, returning"); return; } @@ -565,7 +565,7 @@ public void StartEvent(string key) /// public void CancelEvent(string key) { - if (Configuration.backendMode) { + if (Configuration?.backendMode ?? false) { UtilityHelper.CountlyLogging("[CountlyBase] CancelEvent, Backend Mode enabled, returning"); return; } @@ -607,7 +607,7 @@ private void CancelAllTimedEvents() /// public async Task EndEvent(string key, Segmentation segmentation = null, int count = 1, double? sum = 0) { - if (Configuration.backendMode) { + if (Configuration?.backendMode ?? false) { UtilityHelper.CountlyLogging("[CountlyBase] EndEvent, Backend Mode enabled, returning"); return; } @@ -710,14 +710,14 @@ public static Task RecordEvent(string Key, int Count, double? Sum, Segment /// True if event is uploaded successfully, False - queued for delayed upload public static Task RecordEvent(string Key, int Count, double? Sum, double? Duration, Segmentation Segmentation) { - if (Countly.Instance.Configuration.backendMode) { + if (Countly.Instance.Configuration?.backendMode ?? false) { UtilityHelper.CountlyLogging("[CountlyBase] RecordEvent, Backend Mode enabled, returning false"); - return Task.Factory.StartNew(() => { return false; }); + return BoolTask(false); } if (!Countly.Instance.IsInitialized()) { UtilityHelper.CountlyLogging("SDK must initialized before calling 'RecordEvent'"); - return Task.Factory.StartNew(() => { return false; }); + return BoolTask(false); } CountlyConfig config = Countly.Instance.Configuration; @@ -976,7 +976,7 @@ public static async Task RecordException(string error, string stackTrace, /// True if exception successfully uploaded, False - queued for delayed upload public static async Task RecordException(string error, string stackTrace, Dictionary customInfo, bool unhandled) { - if (Countly.Instance.Configuration.backendMode) { + if (Countly.Instance.Configuration?.backendMode ?? false) { UtilityHelper.CountlyLogging("[CountlyBase] RecordException, Backend Mode enabled, returning false"); return false; } @@ -1337,7 +1337,7 @@ public static void AddBreadCrumb(string log) /// log string public void AddCrashBreadCrumb(string breadCrumb) { - if (Configuration.backendMode) { + if (Configuration?.backendMode ?? false) { UtilityHelper.CountlyLogging("[CountlyBase] AddCrashBreadCrumb, Backend Mode enabled, returning"); return; } @@ -1392,7 +1392,7 @@ protected bool IsAppKeyCorrect(string appKey) public async Task SetLocation(string gpsLocation, string ipAddress = null, string country_code = null, string city = null) { - if (Configuration.backendMode) { + if (Configuration?.backendMode ?? false) { UtilityHelper.CountlyLogging("[CountlyBase] SetLocation, Backend Mode enabled, returning false"); return false; } @@ -1490,7 +1490,7 @@ internal async Task SendRequestWithEmptyLocation() public async Task DisableLocation() { - if (Configuration.backendMode) { + if (Configuration?.backendMode ?? false) { UtilityHelper.CountlyLogging("[CountlyBase] DisableLocation, Backend Mode enabled, returning false"); return false; } @@ -1517,6 +1517,16 @@ internal bool IsInitialized() return false; } + // Returns an already-completed Task with the given value. net35 has no Task.FromResult, + // so we complete a TaskCompletionSource explicitly instead of scheduling a thread-pool + // work item just to return a constant. + private static Task BoolTask(bool value) + { + TaskCompletionSource tcs = new TaskCompletionSource(); + tcs.SetResult(value); + return tcs.Task; + } + /// /// Drops queued requests older than the server-configured 'dort' limit (in hours). /// The request age is read from its embedded 'timestamp' parameter; 0 disables the feature. @@ -1838,7 +1848,7 @@ public async Task SessionEnd() public async Task ChangeDeviceId(string newDeviceId, bool serverSideMerge = false) { - if (Configuration.backendMode) { + if (Configuration?.backendMode ?? false) { UtilityHelper.CountlyLogging("[CountlyBase] ChangeDeviceId, Backend Mode enabled, returning"); return; } @@ -1944,7 +1954,7 @@ internal bool IsConsentGiven(ConsentFeatures feature) public async Task SetConsent(Dictionary consentChanges) { - if (Configuration.backendMode) { + if (Configuration?.backendMode ?? false) { UtilityHelper.CountlyLogging("[CountlyBase] SetConsent, Backend Mode enabled, returning"); return; } @@ -2091,7 +2101,7 @@ internal async Task SendConsentChanges(Dictionary updated /// public async Task RecordView(string viewName) { - if (Configuration.backendMode) { + if (Configuration?.backendMode ?? false) { UtilityHelper.CountlyLogging("[CountlyBase] RecordView, Backend Mode enabled, returning false"); return false; } diff --git a/countlyCommon/countlyCommon/Helpers/TimeHelper.cs b/countlyCommon/countlyCommon/Helpers/TimeHelper.cs index 2e2af197..f5fe082e 100644 --- a/countlyCommon/countlyCommon/Helpers/TimeHelper.cs +++ b/countlyCommon/countlyCommon/Helpers/TimeHelper.cs @@ -64,6 +64,9 @@ internal static TimeInstant Get(long timestampInMillis) //variable to hold last used timestamp private long _lastMilliSecTimeStamp = 0; + //guards the read-modify-write on _lastMilliSecTimeStamp so concurrent callers + //(events/views recorded from multiple threads) never receive the same value + private readonly object timeLock = new object(); internal TimeHelper() { } @@ -82,15 +85,17 @@ public long ToUnixTime(DateTime date) public long GetUniqueUnixTime() { - long calculatedMillis = ToUnixTime(DateTime.Now.ToUniversalTime()); + lock (timeLock) { + long calculatedMillis = ToUnixTime(DateTime.Now.ToUniversalTime()); - if (_lastMilliSecTimeStamp >= calculatedMillis) { - ++_lastMilliSecTimeStamp; - } else { - _lastMilliSecTimeStamp = calculatedMillis; - } + if (_lastMilliSecTimeStamp >= calculatedMillis) { + ++_lastMilliSecTimeStamp; + } else { + _lastMilliSecTimeStamp = calculatedMillis; + } - return _lastMilliSecTimeStamp; + return _lastMilliSecTimeStamp; + } } public TimeInstant GetUniqueInstant() diff --git a/countlyCommon/countlyCommon/Server/ApiBase.cs b/countlyCommon/countlyCommon/Server/ApiBase.cs index 27997496..90b0ffab 100644 --- a/countlyCommon/countlyCommon/Server/ApiBase.cs +++ b/countlyCommon/countlyCommon/Server/ApiBase.cs @@ -115,7 +115,7 @@ protected async Task CallJob(string address, string requestData, if (requestData.StartsWith("/i?")) { // for migrating old requests requestData = requestData.Replace("/i?", ""); } - requestData = AddChekcsum(requestData); + requestData = AddChecksum(requestData); UtilityHelper.CountlyLogging(string.Format("[ApiBase] CallJob, address: [{0}], endpoint: [{1}] requestData: [{2}]", address, endpoint, requestData)); try { @@ -141,7 +141,7 @@ protected async Task CallJob(string address, string requestData, return await tcs.Task; } - private string AddChekcsum(string data) + private string AddChecksum(string data) { if (tamperingProtectionSalt == null || tamperingProtectionSalt.Length == 0) { return data; diff --git a/countlyCommon/countlyCommon/Server/Responses/RequestResult.cs b/countlyCommon/countlyCommon/Server/Responses/RequestResult.cs index 49e34717..e15a5db7 100644 --- a/countlyCommon/countlyCommon/Server/Responses/RequestResult.cs +++ b/countlyCommon/countlyCommon/Server/Responses/RequestResult.cs @@ -20,7 +20,9 @@ public bool IsSuccess() try { return JObject.Parse(responseText).ContainsKey("result"); - } catch (Exception e) { } + } catch (Exception ex) { + CountlySDK.Helpers.UtilityHelper.CountlyLogging("[RequestResult] IsSuccess, could not parse response as JSON: " + ex.Message); + } return false; } diff --git a/net35/Countly/Server/Api.cs b/net35/Countly/Server/Api.cs index a7e5105a..87338be7 100644 --- a/net35/Countly/Server/Api.cs +++ b/net35/Countly/Server/Api.cs @@ -79,10 +79,27 @@ protected override async Task RequestAsync(string address, String } } - var response = (HttpWebResponse)request.GetResponse(); - requestResult.responseCode = (int)response.StatusCode; - requestResult.responseText = new StreamReader(response.GetResponseStream()).ReadToEnd(); + using (var response = (HttpWebResponse)request.GetResponse()) + using (var responseStream = response.GetResponseStream()) + using (var reader = new StreamReader(responseStream)) { + requestResult.responseCode = (int)response.StatusCode; + requestResult.responseText = reader.ReadToEnd(); + } + return requestResult; + } catch (WebException wex) { + // GetResponse() throws on 4xx/5xx. Recover the real status code and body from + // the exception's response instead of leaving responseCode at -1. + UtilityHelper.CountlyLogging("Encountered a WebException while making a POST request, " + wex.ToString()); + HttpWebResponse errorResponse = wex.Response as HttpWebResponse; + if (errorResponse != null) { + using (errorResponse) + using (var errorStream = errorResponse.GetResponseStream()) + using (var errorReader = new StreamReader(errorStream)) { + requestResult.responseCode = (int)errorResponse.StatusCode; + requestResult.responseText = errorReader.ReadToEnd(); + } + } return requestResult; } catch (Exception ex) { UtilityHelper.CountlyLogging("Encountered a exception while making a POST request, " + ex.ToString()); diff --git a/net45/Countly/Server/Api.cs b/net45/Countly/Server/Api.cs index 80896c2b..6e997145 100644 --- a/net45/Countly/Server/Api.cs +++ b/net45/Countly/Server/Api.cs @@ -80,10 +80,27 @@ protected override async Task RequestAsync(string address, String } } - var response = (HttpWebResponse)request.GetResponse(); - requestResult.responseCode = (int)response.StatusCode; - requestResult.responseText = new StreamReader(response.GetResponseStream()).ReadToEnd(); + using (var response = (HttpWebResponse)request.GetResponse()) + using (var responseStream = response.GetResponseStream()) + using (var reader = new StreamReader(responseStream)) { + requestResult.responseCode = (int)response.StatusCode; + requestResult.responseText = reader.ReadToEnd(); + } + return requestResult; + } catch (WebException wex) { + // GetResponse() throws on 4xx/5xx. Recover the real status code and body from + // the exception's response instead of leaving responseCode at -1. + UtilityHelper.CountlyLogging("Encountered a WebException while making a POST request, " + wex.ToString()); + HttpWebResponse errorResponse = wex.Response as HttpWebResponse; + if (errorResponse != null) { + using (errorResponse) + using (var errorStream = errorResponse.GetResponseStream()) + using (var errorReader = new StreamReader(errorStream)) { + requestResult.responseCode = (int)errorResponse.StatusCode; + requestResult.responseText = errorReader.ReadToEnd(); + } + } return requestResult; } catch (Exception ex) { UtilityHelper.CountlyLogging("Encountered a exception while making a POST request, " + ex.ToString()); diff --git a/netstd/Countly/Server/Api.cs b/netstd/Countly/Server/Api.cs index 13f77834..f3a0f16c 100644 --- a/netstd/Countly/Server/Api.cs +++ b/netstd/Countly/Server/Api.cs @@ -25,6 +25,12 @@ internal Api() { } public static Api Instance { get { return instance; } } //-------------SINGLETON----------------- + // Shared across all requests. Constructing an HttpClient per request exhausts sockets + // under load, because each instance holds its own connection pool that lingers in + // TIME_WAIT. Per-request data (custom headers) is applied to the HttpContent, so this + // client carries no per-request state and is safe to reuse. + private static readonly HttpClient httpClient = new HttpClient { Timeout = TimeSpan.FromSeconds(60) }; + protected override async Task Call(string address, string requestData, Stream imageData = null, string endpoint = sdkEndpoint) { return await Task.Run(async () => { @@ -52,7 +58,6 @@ protected override async Task RequestAsync(string address, string } } - HttpClient httpClient = new HttpClient(); HttpResponseMessage httpResponseMessage = await httpClient.PostAsync(address, httpContent); requestResult.responseText = await httpResponseMessage.Content.ReadAsStringAsync(); diff --git a/netstd/CountlyTest_461/CountlyTest_461.csproj b/netstd/CountlyTest_461/CountlyTest_461.csproj index 772be4a6..54e6ee25 100644 --- a/netstd/CountlyTest_461/CountlyTest_461.csproj +++ b/netstd/CountlyTest_461/CountlyTest_461.csproj @@ -133,6 +133,12 @@ ViewsTests.cs + + TimeHelperTests.cs + + + PreInitSafetyTests.cs +