Fix memory leak in Webflux related to an ever growing stack #2580

adinauer · 2023-03-02T04:41:25Z

📜 Description

Use a new hub cloned from main hub for WebFlux requests instead of cloning the current one on the thread. This allows the hub, used for the request, to be garbage collected eventually. Previously we were using the current hub on the thread to call .pushScope(), then cloned that hub including the stack and the call to .popScope() would go to the clone instead of the hub we called .pushScope() on. This means we were popping the copied stack of the cloned hub leaving the stack of the original hub untouched. This lead to an ever growing stack on the original hub, causing a memory leak and also scope bleed because the original hub survives across requests and is never cleaned up.

💡 Motivation and Context

Improves #2557 but still holds on to one hub per thread + stack and scope etc.
I have some ideas for further improving this which I'll try later. I'd like to first ship this as it already makes the situation a lot better.

💚 How did you test it?

Manually running our sample and looking at visualvm as well as debugging thread locals.

  @GetMapping("{id}")
  Mono<Person> person(@PathVariable Long id) {
//    LOGGER.info("Loading person with id={}", id);
//    throw new IllegalArgumentException("Something went wrong [id=" + id + "]");
    return personService.create(new Person("f", "l")).flatMap(it -> Mono.error(new RuntimeException("oopsie from webflux")));
  }

wrk -t2 -c100 -d30s http://user:password@localhost:8080/person/91

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

github-actions · 2023-03-02T04:42:35Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 6e9681d

adinauer · 2023-03-02T04:42:02Z

...s-spring-boot-webflux/src/main/java/io/sentry/samples/spring/boot/SentryDemoApplication.java


 @SpringBootApplication
 public class SentryDemoApplication {
  public static void main(String[] args) {
    SpringApplication.run(SentryDemoApplication.class, args);
  }
+
+  @Bean


copied from other samples

adinauer · 2023-03-02T04:42:11Z

...les/sentry-samples-spring-boot-webflux/src/main/java/io/sentry/samples/spring/boot/Todo.java

@@ -0,0 +1,25 @@
+package io.sentry.samples.spring.boot;
+
+public class Todo {


copied from other samples

adinauer · 2023-03-02T04:42:17Z

...-samples-spring-boot-webflux/src/main/java/io/sentry/samples/spring/boot/TodoController.java

+import org.springframework.web.reactive.function.client.WebClient;
+import reactor.core.publisher.Mono;
+
+@RestController


copied from other samples

adinauer · 2023-03-02T04:45:00Z

sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilter.java

  }

  @Override
  public Mono<Void> filter(
      final @NotNull ServerWebExchange serverWebExchange,
      final @NotNull WebFilterChain webFilterChain) {
+    @NotNull IHub requestHub = Sentry.cloneMainHub();


We now clone the hub from the main hub instead of using whatever hub is present on the thread.

adinauer · 2023-03-02T04:46:42Z

sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilter.java

    return webFilterChain
        .filter(serverWebExchange)
        .doFinally(
            __ -> {
-              hub.popScope();
+              doFinally(requestHub);
+              Sentry.setCurrentHub(NoOpHub.getInstance());


This only resets one of the ThreadLocals. In testing the thread were .doFirst is run still keeps the hub alive. Don't know of a way to clean the ThreadLocal (yet) but I have another idea i wanna explore so we only leak an id and reference the hub in a store instead. I wanna try that in a different PR.

adinauer · 2023-03-02T04:49:44Z

sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebFilter.java

            })
        .doFirst(
            () -> {
-              hub.pushScope();
+              Sentry.setCurrentHub(requestHub);


Need to set it as current hub for the thread as SentryScheduleHook clones the current hub and without this it'd clone a random hub breaking scope.

github-actions · 2023-03-02T04:55:06Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	347.54 ms	394.89 ms	47.35 ms
Size	1.73 MiB	2.34 MiB	626.22 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`db5bd4e`	352.90 ms	359.52 ms	6.62 ms
`d691d8f`	352.56 ms	389.59 ms	37.03 ms
`db5bd4e`	314.82 ms	349.17 ms	34.35 ms
`db5bd4e`	347.00 ms	366.74 ms	19.74 ms
`db5bd4e`	286.94 ms	315.26 ms	28.32 ms
`e5bbb00`	317.46 ms	353.26 ms	35.80 ms

App size

Revision	Plain	With Sentry	Diff
`db5bd4e`	1.73 MiB	2.34 MiB	626.30 KiB
`d691d8f`	1.73 MiB	2.34 MiB	626.23 KiB
`db5bd4e`	1.73 MiB	2.34 MiB	626.30 KiB
`db5bd4e`	1.73 MiB	2.34 MiB	626.30 KiB
`db5bd4e`	1.73 MiB	2.34 MiB	626.30 KiB
`e5bbb00`	1.73 MiB	2.34 MiB	626.22 KiB

Previous results on branch: feat/new-hub-for-webflux-request

Startup times

Revision	Plain	With Sentry	Diff
`303fe40`	286.78 ms	320.36 ms	33.58 ms

App size

Revision	Plain	With Sentry	Diff
`303fe40`	1.73 MiB	2.34 MiB	626.22 KiB

codecov · 2023-03-02T05:06:28Z

Codecov Report

Patch coverage: 76.19% and project coverage change: -0.02 ⚠️

Comparison is base (e5bbb00) 80.44% compared to head (6e9681d) 80.42%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2580      +/-   ##
============================================
- Coverage     80.44%   80.42%   -0.02%     
- Complexity     4020     4024       +4     
============================================
  Files           332      333       +1     
  Lines         15160    15181      +21     
  Branches       1979     1979              
============================================
+ Hits          12195    12210      +15     
- Misses         2186     2192       +6     
  Partials        779      779

Impacted Files	Coverage Δ
...ebflux/SentryWebFilterWithThreadLocalAccessor.java	`0.00% <0.00%> (ø)`
...pring/jakarta/webflux/AbstractSentryWebFilter.java	`94.44% <94.44%> (ø)`
...sentry/spring/jakarta/webflux/SentryWebFilter.java	`100.00% <100.00%> (+4.54%)`	⬆️
...java/io/sentry/spring/webflux/SentryWebFilter.java	`96.00% <100.00%> (+0.76%)`	⬆️
sentry/src/main/java/io/sentry/HubAdapter.java	`9.23% <0.00%> (+3.07%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Fix memory leak in Webflux (Spring Boot 2)

a10eeb7

adinauer requested review from romtsn, stefanosiano and markushi as code owners March 2, 2023 04:41

adinauer commented Mar 2, 2023

View reviewed changes

add changelog

6e9681d

antonpirker approved these changes Mar 2, 2023

View reviewed changes

adinauer merged commit 1345e5d into main Mar 2, 2023

adinauer deleted the feat/new-hub-for-webflux-request branch March 2, 2023 10:10

adinauer mentioned this pull request Mar 2, 2023

Spring Boot (Webflux): many unrelated http breadcrumbs are recorded with events #1790

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak in Webflux related to an ever growing stack #2580

Fix memory leak in Webflux related to an ever growing stack #2580

adinauer commented Mar 2, 2023 •

edited

Loading

github-actions bot commented Mar 2, 2023 •

edited

Loading

adinauer Mar 2, 2023

adinauer Mar 2, 2023

adinauer Mar 2, 2023

adinauer Mar 2, 2023

adinauer Mar 2, 2023

adinauer Mar 2, 2023

github-actions bot commented Mar 2, 2023 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/new-hub-for-webflux-request

Startup times

App size

codecov bot commented Mar 2, 2023 •

edited

Loading

		@@ -0,0 +1,25 @@
		package io.sentry.samples.spring.boot;

		public class Todo {

Fix memory leak in Webflux related to an ever growing stack #2580

Fix memory leak in Webflux related to an ever growing stack #2580

Conversation

adinauer commented Mar 2, 2023 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Mar 2, 2023 • edited Loading

adinauer Mar 2, 2023

Choose a reason for hiding this comment

adinauer Mar 2, 2023

Choose a reason for hiding this comment

adinauer Mar 2, 2023

Choose a reason for hiding this comment

adinauer Mar 2, 2023

Choose a reason for hiding this comment

adinauer Mar 2, 2023

Choose a reason for hiding this comment

adinauer Mar 2, 2023

Choose a reason for hiding this comment

github-actions bot commented Mar 2, 2023 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/new-hub-for-webflux-request

Startup times

App size

codecov bot commented Mar 2, 2023 • edited Loading

Codecov Report

adinauer commented Mar 2, 2023 •

edited

Loading

github-actions bot commented Mar 2, 2023 •

edited

Loading

github-actions bot commented Mar 2, 2023 •

edited

Loading

codecov bot commented Mar 2, 2023 •

edited

Loading