Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Mar 2, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a10eeb7
Fix memory leak in Webflux (Spring Boot 2)
adinauer Mar 1, 2023
6e9681d
add changelog
adinauer Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...pring-boot-webflux/src/main/java/io/sentry/samples/spring/boot/SentryDemoApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,17 @@

import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.context.annotation.Bean;
import org.springframework.web.reactive.function.client.WebClient;

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

@Bean
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copied from other samples

WebClient webClient(WebClient.Builder builder) {
return builder.build();
}
}
25 changes: 25 additions & 0 deletions .../sentry-samples-spring-boot-webflux/src/main/java/io/sentry/samples/spring/boot/Todo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package io.sentry.samples.spring.boot;

public class Todo {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copied from other samples

private final Long id;
private final String title;
private final boolean completed;

public Todo(Long id, String title, boolean completed) {
this.id = id;
this.title = title;
this.completed = completed;
}

public Long getId() {
return id;
}

public String getTitle() {
return title;
}

public boolean isCompleted() {
return completed;
}
}
26 changes: 26 additions & 0 deletions ...mples-spring-boot-webflux/src/main/java/io/sentry/samples/spring/boot/TodoController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package io.sentry.samples.spring.boot;

import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.reactive.function.client.WebClient;
import reactor.core.publisher.Mono;

@RestController
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copied from other samples

public class TodoController {
private final WebClient webClient;

public TodoController(WebClient webClient) {
this.webClient = webClient;
}

@GetMapping("/todo-webclient/{id}")
Mono<Todo> todoWebClient(@PathVariable Long id) {
return webClient
.get()
.uri("https://jsonplaceholder.typicode.com/todos/{id}", id)
.retrieve()
.bodyToMono(Todo.class)
.map(response -> response);
}
}
52 changes: 52 additions & 0 deletions ...pring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/AbstractSentryWebFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package io.sentry.spring.jakarta.webflux;

import com.jakewharton.nopen.annotation.Open;

import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;
import org.springframework.http.server.reactive.ServerHttpRequest;
import org.springframework.http.server.reactive.ServerHttpResponse;
import org.springframework.web.server.ServerWebExchange;
import org.springframework.web.server.WebFilter;
import org.springframework.web.server.WebFilterChain;

import io.sentry.Breadcrumb;
import io.sentry.Hint;
import io.sentry.IHub;
import io.sentry.Sentry;
import static io.sentry.TypeCheckHint.WEBFLUX_FILTER_REQUEST;
import static io.sentry.TypeCheckHint.WEBFLUX_FILTER_RESPONSE;
import io.sentry.util.Objects;
import reactor.core.publisher.Mono;

/** Manages {@link io.sentry.Scope} in Webflux request processing. */
@ApiStatus.Experimental
public abstract class AbstractSentryWebFilter implements WebFilter {
private final @NotNull SentryRequestResolver sentryRequestResolver;
public static final String SENTRY_HUB_KEY = "sentry-hub";

public AbstractSentryWebFilter(final @NotNull IHub hub) {
Objects.requireNonNull(hub, "hub is required");
this.sentryRequestResolver = new SentryRequestResolver(hub);
}

protected void doFinally(final @NotNull IHub requestHub) {
requestHub.popScope();
}

protected void doFirst(final @NotNull ServerWebExchange serverWebExchange, final @NotNull IHub requestHub) {
serverWebExchange.getAttributes().put(SENTRY_HUB_KEY, requestHub);
requestHub.pushScope();
final ServerHttpRequest request = serverWebExchange.getRequest();
final ServerHttpResponse response = serverWebExchange.getResponse();

final Hint hint = new Hint();
hint.set(WEBFLUX_FILTER_REQUEST, request);
hint.set(WEBFLUX_FILTER_RESPONSE, response);
final String methodName =
request.getMethod() != null ? request.getMethod().name() : "unknown";
requestHub.addBreadcrumb(Breadcrumb.http(request.getURI().toString(), methodName), hint);
requestHub.configureScope(
scope -> scope.setRequest(sentryRequestResolver.resolveSentryRequest(request)));
}
}
28 changes: 8 additions & 20 deletions sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.jakewharton.nopen.annotation.Open;

import io.sentry.NoOpHub;
import io.sentry.Sentry;
import static io.sentry.TypeCheckHint.WEBFLUX_FILTER_REQUEST;
import static io.sentry.TypeCheckHint.WEBFLUX_FILTER_RESPONSE;
Expand All @@ -22,41 +23,28 @@
/** Manages {@link io.sentry.Scope} in Webflux request processing. */
@ApiStatus.Experimental
@Open
public class SentryWebFilter implements WebFilter {
private final @NotNull IHub hub;
private final @NotNull SentryRequestResolver sentryRequestResolver;
public static final String SENTRY_HUB_KEY = "sentry-hub";
public class SentryWebFilter extends AbstractSentryWebFilter {

public SentryWebFilter(final @NotNull IHub hub) {
this.hub = Objects.requireNonNull(hub, "hub is required");
this.sentryRequestResolver = new SentryRequestResolver(hub);
super(hub);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})
.doFirst(
() -> {
serverWebExchange.getAttributes().put(SENTRY_HUB_KEY, Sentry.getCurrentHub());
hub.pushScope();
final ServerHttpRequest request = serverWebExchange.getRequest();
final ServerHttpResponse response = serverWebExchange.getResponse();

final Hint hint = new Hint();
hint.set(WEBFLUX_FILTER_REQUEST, request);
hint.set(WEBFLUX_FILTER_RESPONSE, response);
final String methodName =
request.getMethod() != null ? request.getMethod().name() : "unknown";
hub.addBreadcrumb(Breadcrumb.http(request.getURI().toString(), methodName), hint);
hub.configureScope(
scope -> scope.setRequest(sentryRequestResolver.resolveSentryRequest(request)));
Sentry.setCurrentHub(requestHub);
doFirst(serverWebExchange, requestHub);
});
}
}
16 changes: 14 additions & 2 deletions ...rc/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilterWithThreadLocalAccessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@
import org.springframework.web.server.WebFilterChain;

import io.sentry.IHub;
import io.sentry.NoOpHub;
import io.sentry.Sentry;
import reactor.core.publisher.Mono;

/** Manages {@link io.sentry.Scope} in Webflux request processing. */
@ApiStatus.Experimental
public final class SentryWebFilterWithThreadLocalAccessor extends SentryWebFilter {
public final class SentryWebFilterWithThreadLocalAccessor extends AbstractSentryWebFilter {

public SentryWebFilterWithThreadLocalAccessor(final @NotNull IHub hub) {
super(hub);
Expand All @@ -20,6 +22,16 @@ public SentryWebFilterWithThreadLocalAccessor(final @NotNull IHub hub) {
public Mono<Void> filter(
final @NotNull ServerWebExchange serverWebExchange,
final @NotNull WebFilterChain webFilterChain) {
return ReactorUtils.withSentryNewMainHubClone(super.filter(serverWebExchange, webFilterChain));
return ReactorUtils.withSentryNewMainHubClone(webFilterChain
.filter(serverWebExchange)
.doFinally(
__ -> {
doFinally(Sentry.getCurrentHub());
Sentry.setCurrentHub(NoOpHub.getInstance());
})
.doFirst(
() -> {
doFirst(serverWebExchange, Sentry.getCurrentHub());
}));
}
}
18 changes: 12 additions & 6 deletions sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import io.sentry.Breadcrumb;
import io.sentry.Hint;
import io.sentry.IHub;
import io.sentry.NoOpHub;
import io.sentry.Sentry;
import io.sentry.util.Objects;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;
Expand All @@ -19,27 +21,30 @@
/** Manages {@link io.sentry.Scope} in Webflux request processing. */
@ApiStatus.Experimental
public final class SentryWebFilter implements WebFilter {
private final @NotNull IHub hub;

private final @NotNull SentryRequestResolver sentryRequestResolver;

public SentryWebFilter(final @NotNull IHub hub) {
this.hub = Objects.requireNonNull(hub, "hub is required");
Objects.requireNonNull(hub, "hub is required");
this.sentryRequestResolver = new SentryRequestResolver(hub);
}

@Override
public Mono<Void> filter(
final @NotNull ServerWebExchange serverWebExchange,
final @NotNull WebFilterChain webFilterChain) {
@NotNull IHub requestHub = Sentry.cloneMainHub();
return webFilterChain
.filter(serverWebExchange)
.doFinally(
__ -> {
hub.popScope();
requestHub.popScope();
Sentry.setCurrentHub(NoOpHub.getInstance());
})
.doFirst(
() -> {
hub.pushScope();
Sentry.setCurrentHub(requestHub);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

requestHub.pushScope();
final ServerHttpRequest request = serverWebExchange.getRequest();
final ServerHttpResponse response = serverWebExchange.getResponse();

Expand All @@ -48,8 +53,9 @@ public Mono<Void> filter(
hint.set(WEBFLUX_FILTER_RESPONSE, response);
final String methodName =
request.getMethod() != null ? request.getMethod().name() : "unknown";
hub.addBreadcrumb(Breadcrumb.http(request.getURI().toString(), methodName), hint);
hub.configureScope(
requestHub.addBreadcrumb(
Breadcrumb.http(request.getURI().toString(), methodName), hint);
requestHub.configureScope(
scope -> scope.setRequest(sentryRequestResolver.resolveSentryRequest(request)));
});
}
Expand Down