Merge pull request #122 from bugsnag/fix-session-counts

[PLAT-2139] Ensure session counts are reported thread safe
bugsnag · Nov 29, 2018 · 5449cd4 · 5449cd4
2 parents 5e294cf + 3cfe6b6
commit 5449cd4
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,7 +1,10 @@
 # Changelog
 
-## 3.4.2 (2018-11-28)
+## 3.4.2 (2018-11-29)
 
+* Ensure session counts are thread safe
+  [#122](https://github.com/bugsnag/bugsnag-java/pull/122)
+
  * Prevent application hangs due to session flushing
 [#121](https://github.com/bugsnag/bugsnag-java/pull/121)
 

diff --git a/bugsnag/src/main/java/com/bugsnag/Report.java b/bugsnag/src/main/java/com/bugsnag/Report.java
@@ -23,7 +23,7 @@ public class Report {
     private String groupingHash;
     private Diagnostics diagnostics;
     private boolean shouldCancel = false;
-    private Session session;
+    private Map<String, Object> sessionMap;
     private final List<ThreadState> threadStates;
 
     /**
@@ -131,23 +131,22 @@ public Map<String, Object> getMetaData() {
 
     @Expose
     Map<String, Object> getSession() {
-        if (session == null) {
-            return null;
-        }
-
-        Map<String, Object> map = new HashMap<String, Object>();
-        map.put("id", session.getId());
-        map.put("startedAt", session.getStartedAt());
-
-        Map<String, Object> handledCounts = new HashMap<String, Object>();
-        handledCounts.put("handled", session.getHandledCount());
-        handledCounts.put("unhandled", session.getUnhandledCount());
-        map.put("events", handledCounts);
-        return map;
+        return sessionMap;
     }
 
     void setSession(Session session) {
-        this.session = session;
+        if (session == null) {
+            sessionMap = null;
+        } else {
+            sessionMap = new HashMap<String, Object>();
+            sessionMap.put("id", session.getId());
+            sessionMap.put("startedAt", session.getStartedAt());
+
+            Map<String, Object> handledCounts = new HashMap<String, Object>();
+            handledCounts.put("handled", session.getHandledCount());
+            handledCounts.put("unhandled", session.getUnhandledCount());
+            sessionMap.put("events", handledCounts);
+        }
     }
 
     /**

diff --git a/bugsnag/src/test/java/com/bugsnag/BugsnagTest.java b/bugsnag/src/test/java/com/bugsnag/BugsnagTest.java
@@ -510,6 +510,29 @@ public void close() {
         assertTrue(bugsnag.notify(new Throwable()));
     }
 
+    @Test
+    public void testMultipleHandledIncrementWithSession() {
+        bugsnag.startSession();
+        StubNotificationDelivery testDelivery = new StubNotificationDelivery();
+        bugsnag.setDelivery(testDelivery);
+
+        assertTrue(bugsnag.notify(new Throwable()));
+        assertTrue(bugsnag.notify(new Throwable()));
+        assertTrue(bugsnag.notify(new Throwable()));
+
+        for (int i = 0 ; i < testDelivery.getNotifications().size(); i++) {
+            Report report = testDelivery.getNotifications().get(i).getEvents().get(0);
+
+            Map<String, Object> session = report.getSession();
+            assertNotNull(session);
+
+            @SuppressWarnings("unchecked")
+            Map<String, Object> handledCounts = (Map<String, Object>) session.get("events");
+            assertEquals(i + 1, handledCounts.get("handled"));
+            assertEquals(0, handledCounts.get("unhandled"));
+        }
+    }
+
     @Test
     public void testSerialization() {
         ByteArrayOutputStream byteStream = new ByteArrayOutputStream();

diff --git a/bugsnag/src/test/java/com/bugsnag/NotificationTest.java b/bugsnag/src/test/java/com/bugsnag/NotificationTest.java
@@ -5,6 +5,7 @@
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 
+import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.junit.Before;
@@ -31,6 +32,9 @@ public void setUp() {
         config.appVersion = "1.2.3";
         config.releaseStage = "dev";
         report = new Report(config, new RuntimeException());
+
+        // Only include properties with non-null values
+        mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
     }
 
     private JsonNode generateJson(ObjectMapper mapper,
@@ -67,6 +71,17 @@ public void testWithoutSessionSerialisation() throws Throwable {
         assertNull(rootNode.get("events").get("session"));
     }
 
+    @Test
+    public void testNullSession() throws Throwable {
+        report.setSession(null);
+
+        JsonNode rootNode = generateJson(mapper, config, report);
+        validateErrorReport(rootNode);
+
+        JsonNode session = rootNode.get("events").get(0).get("session");
+        assertNull(session);
+    }
+
     private void validateErrorReport(JsonNode rootNode) {
         assertNotNull(rootNode);
         assertNotNull(rootNode.get("apiKey").asText());