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.

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

Virtual thread pinning on MainEventProcessor.ensureHostnameCache #3312

Closed
pavelbelonosov opened this issue Apr 2, 2024 · 4 comments
Closed
Assignees
Milestone

Comments

@pavelbelonosov
Copy link

Problem Statement

We use Java 21 and Spring Boot 3.2.4 with enabled virtual threads in plain web application. First request after app init leads to thread pinning: io.sentry.MainEventProcessor.ensureHostnameCache(MainEventProcessor.java:192) <== monitors:1. Subsequent requests are fine.

Thread[#143,ForkJoinPool-1-worker-1,5,CarrierThreads]
    java.base/java.lang.VirtualThread$VThreadContinuation.onPinned(VirtualThread.java:185)
    java.base/jdk.internal.vm.Continuation.onPinned0(Continuation.java:393)
    java.base/java.lang.VirtualThread.parkNanos(VirtualThread.java:631)
    java.base/java.lang.System$2.parkVirtualThread(System.java:2648)
    java.base/jdk.internal.misc.VirtualThreads.park(VirtualThreads.java:67)
    java.base/java.util.concurrent.locks.LockSupport.parkNanos(LockSupport.java:267)
    java.base/java.util.concurrent.FutureTask.awaitDone(FutureTask.java:497)
    java.base/java.util.concurrent.FutureTask.get(FutureTask.java:203)
    io.sentry.HostnameCache.updateCache(HostnameCache.java:121)
    io.sentry.HostnameCache.<init>(HostnameCache.java:77)
    io.sentry.HostnameCache.<init>(HostnameCache.java:64)
    io.sentry.HostnameCache.<init>(HostnameCache.java:58)
    io.sentry.HostnameCache.getInstance(HostnameCache.java:52)
    io.sentry.MainEventProcessor.ensureHostnameCache(MainEventProcessor.java:192) <== monitors:1
    io.sentry.MainEventProcessor.setServerName(MainEventProcessor.java:181)
    io.sentry.MainEventProcessor.processNonCachedEvent(MainEventProcessor.java:132)
    io.sentry.MainEventProcessor.process(MainEventProcessor.java:146)
    io.sentry.SentryClient.processTransaction(SentryClient.java:416)
    io.sentry.SentryClient.captureTransaction(SentryClient.java:661)
    io.sentry.Hub.captureTransaction(Hub.java:700)
    io.sentry.SentryTracer.finish(SentryTracer.java:264)
    io.sentry.SentryTracer.finish(SentryTracer.java:565)
    io.sentry.SentryTracer.finish(SentryTracer.java:558)
    io.sentry.SentryTracer.finish(SentryTracer.java:553)
    io.sentry.spring.jakarta.tracing.SentryTracingFilter.doFilterWithTransaction(SentryTracingFilter.java:127)
    io.sentry.spring.jakarta.tracing.SentryTracingFilter.doFilterInternal(SentryTracingFilter.java:87)
    org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
    org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:174)
    org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:149)
    org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:201)
    org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
    org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:174)
    org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:149)
    io.sentry.spring.jakarta.SentrySpringFilter.doFilterInternal(SentrySpringFilter.java:71)
    org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
    org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:174)
    org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:149)
    org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:167)
    org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:90)
    org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:482)
    org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:115)
    org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:93)
    org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74)
    org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:344)
    org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:391)
    org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:63)
    org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:896)
    org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1744)
    org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:52)
    java.base/java.lang.VirtualThread.run(VirtualThread.java:311)

This behavior is observed in Sentry 7.6.0 and 6.26.0

Solution Brainstorm

Would be nice if Sentry became Loom-friendly and replace synchronized methods to ReentrantLock for virtual thread support.

From this

private void ensureHostnameCache() {
    if (hostnameCache == null) {
      synchronized (this) {
        if (hostnameCache == null) {
          hostnameCache = HostnameCache.getInstance();
        }
      }
    }
  }

To this:

    private final ReentrantLock lock = new ReentrantLock();

    private void ensureHostnameCache() {
        if (hostnameCache == null) {
            lock.lock();
            try {
                if (hostnameCache == null) {
                    hostnameCache = HostnameCache.getInstance();
                }
            } finally {
                lock.unlock();
            }
        }
    }
@adinauer
Copy link
Member

adinauer commented Apr 5, 2024

Hey @pavelbelonosov thanks for opening this issue. Sounds like a straight forward replacement. Maybe we can fit this into the next major which we're starting to work on - no promises though. We're currently changing a lot of things on the SDK internally, so we can't do it right away or we'd have lots of merge conflicts. ReentrantLock seems to be available for old versions of Android as well.

This class in pgjdbc seems like an interesting approach.

@tkrason
Copy link

tkrason commented Aug 20, 2024

Hello, for any future devs looking at hotfix, we run into similar problem (Spring Boot 3.2.2 / Java 21 / Sentry 7.14.0)

While having virtual threads on, the application would just randomly "stuck", not respond to API calls and also not responding to k8s /actuator/health readyness check, which led me down a rabbit hole of finding what could be wrong.

This behavior was happening completely random, under 0 load on multiple test environents.

Only @Scheduled tasks were running. We use @Scheduled and @SentryTransaction together.

I tracked this to mentioned function ensureHostnameCache(). As soon as I turned attaching server name (and thus calling this function) the problem went away.

The temporary solution for us is to set in application.yaml sentry.attach-server-name: false.

I would thus like to ask very kindly, if it would be possible to merge solution, that would not pin virtual threads.
Thank you very much!

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Aug 20, 2024
@adinauer
Copy link
Member

Thanks @tkrason we'll bump prio on this and try to inclue it in the 8.0 release.

@adinauer
Copy link
Member

Hey @pavelbelonosov and @tkrason we've just released 8.0.0-beta.1 of the Java SDK which replaced synchronized with ReentrantLock. If you decide to give it a try, feedback would be great :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Archived in project
Archived in project
Development

No branches or pull requests

4 participants