code review

sentry-core/src/main/java/io/sentry/core/Scope.java

@@ -392,11 +392,19 @@ public void addEventProcessor(@NotNull EventProcessor eventProcessor) {
    * Callback to do atomic operations on session
    *
    * @param sessionCallback the IWithSession callback
+   * @return a clone of the Session after executing the callback and mutating the session
    */
-  void withSession(@NotNull IWithSession sessionCallback) {
+  @Nullable
+  Session withSession(@NotNull IWithSession sessionCallback) {
+    Session cloneSession = null;
     synchronized (sessionLock) {
       sessionCallback.accept(session);
+
+      if (session != null) {
+        cloneSession = session.clone();
+      }
     }
+    return cloneSession;
   }
 
   /** the IWithSession callback */
@@ -492,20 +500,4 @@ Session endSession() {
     }
     return previousSession;
   }
-
-  /**
-   * Clones a session if there's an active session
-   *
-   * @return the cloned session or null otherwise
-   */
-  @Nullable
-  Session cloneSession() {
-    Session cloneSession = null;
-    synchronized (sessionLock) {
-      if (session != null) {
-        cloneSession = session.clone();
-      }
-    }
-    return cloneSession;
-  }
 }

sentry-core/src/main/java/io/sentry/core/SentryClient.java

@@ -60,8 +60,6 @@ public SentryClient(final @NotNull SentryOptions options, @Nullable Connection c
 
     options.getLogger().log(SentryLevel.DEBUG, "Capturing event: %s", event.getEventId());
 
-    boolean sendEvent = true;
-
     if (ApplyScopeUtils.shouldApplyScopeData(hint)) {
       // Event has already passed through here before it was cached
       // Going through again could be reading data that is no longer relevant
@@ -70,7 +68,6 @@ public SentryClient(final @NotNull SentryOptions options, @Nullable Connection c
 
       if (event == null) {
         options.getLogger().log(SentryLevel.DEBUG, "Event was dropped by applyScope");
-        sendEvent = false;
       }
     } else {
       options
@@ -88,7 +85,6 @@ public SentryClient(final @NotNull SentryOptions options, @Nullable Connection c
                 SentryLevel.DEBUG,
                 "Event was dropped by processor: %s",
                 processor.getClass().getName());
-        sendEvent = false;
         break;
       }
     }
@@ -105,7 +101,7 @@ public SentryClient(final @NotNull SentryOptions options, @Nullable Connection c
                 SentryLevel.DEBUG,
                 "Event %s was dropped due to sampling decision.",
                 event.getEventId());
-        sendEvent = false;
+        // setting event as null to not be sent as its been discarded by sample rate
         event = null;
       }
     }
@@ -115,12 +111,15 @@ public SentryClient(final @NotNull SentryOptions options, @Nullable Connection c
 
       if (event == null) {
         options.getLogger().log(SentryLevel.DEBUG, "Event was dropped by beforeSend");
-        sendEvent = false;
       }
     }
 
     SentryId sentryId = SentryId.EMPTY_ID;
 
+    if (event != null) {
+      sentryId = event.getEventId();
+    }
+
     try {
       final SentryEnvelope envelope = buildEnvelope(event, session);
 
@@ -129,11 +128,9 @@ public SentryClient(final @NotNull SentryOptions options, @Nullable Connection c
       }
     } catch (IOException e) {
       options.getLogger().log(SentryLevel.WARNING, e, "Capturing event %s failed.", sentryId);
-      sendEvent = false;
-    }
 
-    if (sendEvent) {
-      sentryId = event.getEventId();
+      // if there was an error capturing the event, we return an emptyId
+      sentryId = SentryId.EMPTY_ID;
     }
 
     return sentryId;
@@ -182,38 +179,40 @@ Session updateSessionData(
 
     if (ApplyScopeUtils.shouldApplyScopeData(hint)) {
       if (scope != null) {
-        scope.withSession(
-            session -> {
-              if (session != null) {
-                Session.State status = null;
-                if (event.isCrashed()) {
-                  status = Session.State.Crashed;
-                }
-
-                boolean crashedOrErrored = false;
-                if (Session.State.Crashed == status || event.isErrored()) {
-                  crashedOrErrored = true;
-                }
-
-                String userAgent = null;
-                if (event.getRequest() != null && event.getRequest().getHeaders() != null) {
-                  if (event.getRequest().getHeaders().containsKey("user-agent")) {
-                    userAgent = event.getRequest().getHeaders().get("user-agent");
-                  }
-                }
-
-                if (session.update(status, userAgent, crashedOrErrored)) {
-                  // if hint is DiskFlushNotification, it means we have an uncaughtException
-                  // and we can end the session.
-                  if (hint instanceof DiskFlushNotification) {
-                    session.end();
+        clonedSession =
+            scope.withSession(
+                session -> {
+                  if (session != null) {
+                    Session.State status = null;
+                    if (event.isCrashed()) {
+                      status = Session.State.Crashed;
+                    }
+
+                    boolean crashedOrErrored = false;
+                    if (Session.State.Crashed == status || event.isErrored()) {
+                      crashedOrErrored = true;
+                    }
+
+                    String userAgent = null;
+                    if (event.getRequest() != null && event.getRequest().getHeaders() != null) {
+                      if (event.getRequest().getHeaders().containsKey("user-agent")) {
+                        userAgent = event.getRequest().getHeaders().get("user-agent");
+                      }
+                    }
+
+                    if (session.update(status, userAgent, crashedOrErrored)) {
+                      // if hint is DiskFlushNotification, it means we have an uncaughtException
+                      // and we can end the session.
+                      if (hint instanceof DiskFlushNotification) {
+                        session.end();
+                      }
+                    }
+                  } else {
+                    options
+                        .getLogger()
+                        .log(SentryLevel.INFO, "Session is null on scope.withSession");
                   }
-                }
-              } else {
-                options.getLogger().log(SentryLevel.INFO, "Session is null on scope.withSession");
-              }
-            });
-        clonedSession = scope.cloneSession();
+                });
       } else {
         options.getLogger().log(SentryLevel.INFO, "Scope is null on client.captureEvent");
       }

sentry-core/src/test/java/io/sentry/core/SentryClientTest.kt

@@ -604,11 +604,10 @@ class SentryClientTest {
     fun `when context property is missing on the event, property from scope contexts is applied`() {
         val sut = fixture.getSut()
 
-        val event = SentryEvent()
         val scope = Scope(fixture.sentryOptions)
         scope.setContexts("key", "value")
         scope.startSession().current
-        sut.captureEvent(event, scope, null)
+        sut.captureEvent(SentryEvent(), scope, null)
         verify(fixture.connection).send(check {
             val event = getEventFromData(it.items.first().data)
             assertEquals("value", event.contexts["key"])
@@ -626,8 +625,8 @@ class SentryClientTest {
         scope.startSession().current
         sut.captureEvent(event, scope, null)
         verify(fixture.connection).send(check {
-            val event = getEventFromData(it.items.first().data)
-            assertEquals("event value", event.contexts["key"])
+            val eventFromData = getEventFromData(it.items.first().data)
+            assertEquals("event value", eventFromData.contexts["key"])
         }, anyOrNull())
     }