Fix: Make sure scope is popped when processing request results in exc…

…eption (#1665)
getsentry · Aug 19, 2021 · a611eb2 · a611eb2
1 parent 41654d8
commit a611eb2
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 20 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 * Feat: Add support for async methods in Spring MVC (#1652)
 * Feat: Add secondary constructor taking IHub to SentryOkHttpInterceptor (#1657)
 * Feat: Merge external map properties (#1656)
+* Fix: Make sure scope is popped when processing request results in exception (#1665)
 
 ## 5.1.0
 

diff --git a/sentry-spring/src/main/java/io/sentry/spring/SentrySpringFilter.java b/sentry-spring/src/main/java/io/sentry/spring/SentrySpringFilter.java
@@ -53,34 +53,40 @@ protected void doFilterInternal(
       hub.pushScope();
       try {
         hub.addBreadcrumb(Breadcrumb.http(request.getRequestURI(), request.getMethod()));
-        hub.configureScope(
-            scope -> {
-              // set basic request information on the scope
-              scope.setRequest(requestResolver.resolveSentryRequest(request));
-              // transaction name is known at the later stage of request processing, thus it cannot
-              // be set on the scope
-              scope.addEventProcessor(new SentryRequestHttpServletRequestProcessor(request));
-              // only if request caches body, add an event processor that sets body on the event
-              // body is not on the scope, to avoid using memory when no event is triggered during
-              // request processing
-              if (request instanceof CachedBodyHttpServletRequest) {
-                scope.addEventProcessor(
-                    new RequestBodyExtractingEventProcessor(request, hub.getOptions()));
-              }
-            });
-      } catch (Exception e) {
-        hub.getOptions()
-            .getLogger()
-            .log(SentryLevel.ERROR, "Failed to set scope for HTTP request", e);
-      } finally {
+        configureScope(request);
         filterChain.doFilter(request, response);
+      } finally {
         hub.popScope();
       }
     } else {
       filterChain.doFilter(servletRequest, response);
     }
   }
 
+  private void configureScope(HttpServletRequest request) {
+    try {
+      hub.configureScope(
+          scope -> {
+            // set basic request information on the scope
+            scope.setRequest(requestResolver.resolveSentryRequest(request));
+            // transaction name is known at the later stage of request processing, thus it cannot
+            // be set on the scope
+            scope.addEventProcessor(new SentryRequestHttpServletRequestProcessor(request));
+            // only if request caches body, add an event processor that sets body on the event
+            // body is not on the scope, to avoid using memory when no event is triggered during
+            // request processing
+            if (request instanceof CachedBodyHttpServletRequest) {
+              scope.addEventProcessor(
+                  new RequestBodyExtractingEventProcessor(request, hub.getOptions()));
+            }
+          });
+    } catch (Exception e) {
+      hub.getOptions()
+          .getLogger()
+          .log(SentryLevel.ERROR, "Failed to set scope for HTTP request", e);
+    }
+  }
+
   private @NotNull HttpServletRequest resolveHttpServletRequest(
       final @NotNull HttpServletRequest request) {
     if (hub.getOptions().isSendDefaultPii()

diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/SentrySpringFilterTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/SentrySpringFilterTest.kt
@@ -24,6 +24,7 @@ import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
 import kotlin.test.assertNull
 import kotlin.test.assertTrue
+import kotlin.test.fail
 import org.assertj.core.api.Assertions
 import org.springframework.http.MediaType
 import org.springframework.mock.web.MockHttpServletRequest
@@ -83,6 +84,19 @@ class SentrySpringFilterTest {
         verify(fixture.hub).popScope()
     }
 
+    @Test
+    fun `pops scope when chain throws`() {
+        val listener = fixture.getSut()
+        whenever(fixture.chain.doFilter(any(), any())).thenThrow(RuntimeException())
+
+        try {
+            listener.doFilter(fixture.request, fixture.response, fixture.chain)
+            fail()
+        } catch (e: Exception) {
+            verify(fixture.hub).popScope()
+        }
+    }
+
     @Test
     fun `attaches basic information from HTTP request to Scope request`() {
         val listener = fixture.getSut(request = MockMvcRequestBuilders