Skip sending empty sessions data payload (#98)

Currently even if there has been no session data generated we still send a payload to Bugsnag. If no session data has been generated then we should skip sending the payload The most substantial change was in the test code. We needed to support providing a timeout to our test server when we are expecting a certain number of requests. This was added so that we could test the scenario above. The functional change made was to check that session data exists before generating the payload to send.
bugsnag · May 26, 2018 · ecfdbef · ecfdbef
1 parent 5adafc7
commit ecfdbef
Show file tree

Hide file tree

Showing 11 changed files with 82 additions and 35 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,10 @@ Changelog
 
 ### Bug fixes
 
+* Only send session data when it exists
+  | [martin308](https://github.com/martin308)
+  | [98](https://github.com/bugsnag/bugsnag-dotnet/pull/98)
+
 * Request context is now available in custom middleware
   | [martin308](https://github.com/martin308)
   | [#91](https://github.com/bugsnag/bugsnag-dotnet/pull/91)

diff --git a/src/Bugsnag/SessionsStore.cs b/src/Bugsnag/SessionsStore.cs
@@ -2,6 +2,7 @@
 using System;
 using System.Collections.Generic;
 using System.Threading;
+using System.Linq;
 
 namespace Bugsnag
 {
@@ -27,7 +28,8 @@ private void SendSessions(object state)
 
       lock (_lock)
       {
-        foreach (var item in _store)
+        // we only care about entries that have session data
+        foreach (var item in _store.Where(d => d.Value.Any()))
         {
           sessionData[item.Key] = new Dictionary<string, long>(item.Value);
           _store[item.Key].Clear();

diff --git a/tests/Bugsnag.AspNet.Core.Tests/UseExceptionHandlerTests.cs b/tests/Bugsnag.AspNet.Core.Tests/UseExceptionHandlerTests.cs
@@ -24,7 +24,7 @@ public Task DisposeAsync()
 
     public async Task InitializeAsync()
     {
-      var bugsnag = new Bugsnag.Tests.TestServer(1);
+      var bugsnag = new Bugsnag.Tests.TestServer();
       bugsnag.Start();
 
       var builder = new WebHostBuilder()
@@ -47,7 +47,7 @@ public async Task InitializeAsync()
       var client = server.CreateClient();
       var response = await client.SendAsync(new System.Net.Http.HttpRequestMessage() { RequestUri = new Uri("/error", UriKind.Relative) });
 
-      var bugsnags = await bugsnag.Requests();
+      var bugsnags = await bugsnag.Requests(1);
 
       BugsnagPayload = bugsnags.First();
     }

diff --git a/tests/Bugsnag.AspNet.Core.Tests/WebHostTests.cs b/tests/Bugsnag.AspNet.Core.Tests/WebHostTests.cs
@@ -30,7 +30,7 @@ public async void TestWithNoExceptions()
     [Fact]
     public async void TestWithDeveloperExceptionPage()
     {
-      var bugsnag = new Bugsnag.Tests.TestServer(1);
+      var bugsnag = new Bugsnag.Tests.TestServer();
       bugsnag.Start();
 
       var builder = new WebHostBuilder()
@@ -46,7 +46,7 @@ public async void TestWithDeveloperExceptionPage()
       var client = server.CreateClient();
       var response = await client.SendAsync(new System.Net.Http.HttpRequestMessage());
 
-      var bugsnags = await bugsnag.Requests();
+      var bugsnags = await bugsnag.Requests(1);
 
       Assert.NotNull(response);
     }

diff --git a/tests/Bugsnag.AspNet.Tests/ClientTests.cs b/tests/Bugsnag.AspNet.Tests/ClientTests.cs
@@ -19,7 +19,7 @@ public Task DisposeAsync()
 
     public async Task InitializeAsync()
     {
-      var server = new TestServer(1);
+      var server = new TestServer();
 
       server.Start();
 
@@ -37,7 +37,7 @@ public async Task InitializeAsync()
         });
       }
 
-      var requests = await server.Requests();
+      var requests = await server.Requests(1);
 
       _request = requests.Single();
     }

diff --git a/tests/Bugsnag.AspNet.WebApi.Tests/WebHostTests.cs b/tests/Bugsnag.AspNet.WebApi.Tests/WebHostTests.cs
@@ -22,7 +22,7 @@ public IHttpActionResult Test()
     [Fact]
     public async void Test()
     {
-      var bugsnagServer = new TestServer(1);
+      var bugsnagServer = new TestServer();
 
       bugsnagServer.Start();
 
@@ -37,7 +37,7 @@ public async void Test()
 
       var response = await client.SendAsync(request);
 
-      var responses = await bugsnagServer.Requests();
+      var responses = await bugsnagServer.Requests(1);
 
       Assert.Single(responses);
       Assert.Contains("Bugsnag is great!", responses.Single());

diff --git a/tests/Bugsnag.Tests.Server/TestServer.cs b/tests/Bugsnag.Tests.Server/TestServer.cs
@@ -17,10 +17,10 @@ public class TestServer
     private readonly RequestCollection<string> _requests;
     private readonly int _port;
 
-    public TestServer(int expectedNumberOfRequests)
+    public TestServer()
     {
       _port = PortAllocations.Instance.NextFreePort();
-      _requests = new RequestCollection<string>(expectedNumberOfRequests);
+      _requests = new RequestCollection<string>();
       _webHost = new WebHostBuilder()
         .ConfigureServices(services => services.AddSingleton(typeof(RequestCollection<string>), _requests))
         .UseStartup<Startup>()
@@ -37,9 +37,15 @@ public void Start()
       _webHost.Start();
     }
 
-    public async Task<IEnumerable<string>> Requests()
+    public async Task<IEnumerable<string>> Requests(int numberOfRequests)
     {
-      var items = await _requests.Items();
+      var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10));
+      return await Requests(numberOfRequests, cts.Token);
+    }
+
+    public async Task<IEnumerable<string>> Requests(int numberOfRequests, CancellationToken token)
+    {
+      var items = await _requests.Items(numberOfRequests, token);
       await _webHost.StopAsync();
       return items;
     }
@@ -88,16 +94,14 @@ public void Configure(IApplicationBuilder app)
 
     class RequestCollection<T>
     {
-      private readonly int _expectedNumberOfRequests;
       private readonly List<T> _requests;
       private readonly object _requestsLock;
       private readonly TaskCompletionSource<List<T>> _taskCompletionSource;
 
-      public RequestCollection(int expectedNumberOfRequests)
+      public RequestCollection()
       {
         _taskCompletionSource = new TaskCompletionSource<List<T>>();
-        _expectedNumberOfRequests = expectedNumberOfRequests;
-        _requests = new List<T>(expectedNumberOfRequests);
+        _requests = new List<T>();
         _requestsLock= new object();
       }
 
@@ -106,17 +110,26 @@ public void Add(T item)
         lock (_requestsLock)
         {
           _requests.Add(item);
-
-          if (_requests.Count >= _expectedNumberOfRequests)
-          {
-            _taskCompletionSource.SetResult(_requests);
-          }
         }
       }
 
-      public Task<List<T>> Items()
+      public Task<List<T>> Items(int numberOfRequests, CancellationToken token)
       {
-        return _taskCompletionSource.Task;
+        return Task.Factory.StartNew(async () =>
+        {
+          while (true)
+          {
+            lock (_requestsLock)
+            {
+              if (_requests.Count >= numberOfRequests)
+              {
+                return new List<T>(_requests);
+              }
+            }
+            token.ThrowIfCancellationRequested();
+            await Task.Delay(TimeSpan.FromSeconds(1));
+          }
+        }, token).Unwrap();
       }
     }
   }

diff --git a/tests/Bugsnag.Tests/ClientTests.cs b/tests/Bugsnag.Tests/ClientTests.cs
@@ -12,7 +12,7 @@ public class ClientTests
     [Fact]
     public async void TestThrownException()
     {
-      var server = new TestServer(1);
+      var server = new TestServer();
 
       server.Start();
 
@@ -36,7 +36,7 @@ public async void TestThrownException()
         client.Notify(e);
       }
 
-      var requests = await server.Requests();
+      var requests = await server.Requests(1);
 
       Assert.Single(requests);
     }
@@ -52,7 +52,7 @@ public Task DisposeAsync()
 
       public async Task InitializeAsync()
       {
-        var server = new TestServer(1);
+        var server = new TestServer();
 
         server.Start();
 
@@ -69,7 +69,7 @@ public async Task InitializeAsync()
 
         TestNotifyMethod(client);
 
-        var requests = await server.Requests();
+        var requests = await server.Requests(1);
 
         BugsnagPayload = requests.Single();
       }

diff --git a/tests/Bugsnag.Tests/SessionTrackingTests.cs b/tests/Bugsnag.Tests/SessionTrackingTests.cs
@@ -1,4 +1,5 @@
 using System;
+using System.Threading;
 using Xunit;
 
 namespace Bugsnag.Tests
@@ -16,17 +17,44 @@ public void CurrentSessionIsNullWithoutBeingSet()
     [Fact]
     public async void CurrentSessionCanBeSet()
     {
-      var server = new TestServer(1);
+      var server = new TestServer();
 
       server.Start();
 
-      var sessionTracking = new SessionTracker(new Configuration("123456") { SessionTrackingInterval = TimeSpan.FromSeconds(5), SessionEndpoint = server.Endpoint });
+      var sessionTracking = new SessionTracker(new Configuration("123456") {
+        SessionEndpoint = server.Endpoint
+      });
 
       sessionTracking.CreateSession();
 
-      var requests = await server.Requests();
+      var requests = await server.Requests(1);
 
       Assert.NotNull(sessionTracking.CurrentSession);
     }
+
+    [Fact]
+    public async void EmptySessionsPayloadsAreNotSent()
+    {
+      var server = new TestServer();
+
+      server.Start();
+
+      var sessionTracking = new SessionTracker(new Configuration("123456") {
+        SessionEndpoint = server.Endpoint
+      });
+
+      sessionTracking.CreateSession();
+
+      // set the cancellation token length long enough so that the session
+      // tracker would attempt to send sessions twice
+      var cts = new CancellationTokenSource(TimeSpan.FromSeconds(130));
+
+      // the cancellation token should throw as we are 'expecting' 2 requests
+      // to be received but we only want one to be sent as the second sessions
+      // request will have no sessions data
+      await Assert.ThrowsAsync<OperationCanceledException>(async () => {
+        await server.Requests(2, cts.Token);
+      });
+    }
   }
 }
diff --git a/tests/Bugsnag.Tests/ThreadQueueDeliveryTests.cs b/tests/Bugsnag.Tests/ThreadQueueDeliveryTests.cs
@@ -13,7 +13,7 @@ public async void Test()
     {
       var numberOfRequests = 500;
 
-      var server = new TestServer(numberOfRequests);
+      var server = new TestServer();
 
       server.Start();
 
@@ -23,7 +23,7 @@ public async void Test()
         ThreadQueueDelivery.Instance.Send(payload);
       }
 
-      var requests = await server.Requests();
+      var requests = await server.Requests(numberOfRequests);
 
       Assert.Equal(numberOfRequests, requests.Count());
     }

diff --git a/tests/Bugsnag.Tests/WebRequestTests.cs b/tests/Bugsnag.Tests/WebRequestTests.cs
@@ -12,7 +12,7 @@ public class WebRequestTests
     public async void Test()
     {
       var numerOfRequests = 1;
-      var server = new TestServer(numerOfRequests);
+      var server = new TestServer();
       server.Start();
 
       var webRequest = new WebRequest();
@@ -23,7 +23,7 @@ public async void Test()
       var response = await Task.Factory.FromAsync((callback, state) => webRequest.BeginSend(server.Endpoint, null, headers, rawPayload, callback, state), webRequest.EndSend, null);
       Assert.Equal(HttpStatusCode.OK, response.HttpStatusCode);
 
-      var requests = await server.Requests();
+      var requests = await server.Requests(numerOfRequests);
 
       Assert.Equal(numerOfRequests, requests.Count());
     }