From 8c915895b313eb4cd6104302160907f527339435 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Sun, 31 Mar 2024 10:42:58 -0400 Subject: [PATCH] Fix VirtualThread pinning with RealSpan and zipkin2.reporter by switching from synchronized to ReentrantLock Signed-off-by: Andriy Redko --- brave/src/main/java/brave/RealSpan.java | 24 +++++++++++++++---- .../main/java/brave/internal/Platform.java | 7 +++++- .../main/java/brave/internal/extra/Extra.java | 10 +++++--- .../java/brave/internal/extra/MapExtra.java | 5 +++- 4 files changed, 37 insertions(+), 9 deletions(-) diff --git a/brave/src/main/java/brave/RealSpan.java b/brave/src/main/java/brave/RealSpan.java index 68f9d2054b..23fa750dfc 100644 --- a/brave/src/main/java/brave/RealSpan.java +++ b/brave/src/main/java/brave/RealSpan.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2020 The OpenZipkin Authors + * Copyright 2013-2024 The OpenZipkin Authors * * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except * in compliance with the License. You may obtain a copy of the License at @@ -13,12 +13,15 @@ */ package brave; +import java.util.concurrent.locks.ReentrantLock; + import brave.handler.MutableSpan; import brave.internal.recorder.PendingSpans; import brave.propagation.TraceContext; /** This wraps the public api and guards access to a mutable span. */ final class RealSpan extends Span { + final ReentrantLock lock = new ReentrantLock(); final TraceContext context; final PendingSpans pendingSpans; final MutableSpan state; @@ -139,17 +142,30 @@ final class RealSpan extends Span { } @Override public void finish(long timestamp) { - synchronized (state) { + lock.lock(); + try { pendingSpans.finish(context, timestamp); + } finally { + lock.unlock(); } } @Override public void abandon() { - pendingSpans.abandon(context); + lock.lock(); + try { + pendingSpans.abandon(context); + } finally { + lock.unlock(); + } } @Override public void flush() { - pendingSpans.flush(context); + lock.lock(); + try { + pendingSpans.flush(context); + } finally { + lock.unlock(); + } } @Override public String toString() { diff --git a/brave/src/main/java/brave/internal/Platform.java b/brave/src/main/java/brave/internal/Platform.java index 2f1744e614..8e552f38ee 100644 --- a/brave/src/main/java/brave/internal/Platform.java +++ b/brave/src/main/java/brave/internal/Platform.java @@ -21,6 +21,7 @@ import java.security.SecureRandom; import java.util.Enumeration; import java.util.Random; +import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Level; import java.util.logging.LogRecord; import java.util.logging.Logger; @@ -35,6 +36,7 @@ public abstract class Platform implements Clock { private static final Platform PLATFORM = findPlatform(); + private final ReentrantLock lock = new ReentrantLock(); volatile String linkLocalIp; /** Returns the same value as {@link System#nanoTime()}. */ @@ -47,10 +49,13 @@ public abstract class Platform implements Clock { @Nullable public String linkLocalIp() { // uses synchronized variant of double-checked locking as getting the endpoint can be expensive if (linkLocalIp != null) return linkLocalIp; - synchronized (this) { + lock.lock(); + try { if (linkLocalIp == null) { linkLocalIp = produceLinkLocalIp(); } + } finally { + lock.unlock(); } return linkLocalIp; } diff --git a/brave/src/main/java/brave/internal/extra/Extra.java b/brave/src/main/java/brave/internal/extra/Extra.java index e6f4b1a3fa..d0e5a11502 100644 --- a/brave/src/main/java/brave/internal/extra/Extra.java +++ b/brave/src/main/java/brave/internal/extra/Extra.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2020 The OpenZipkin Authors + * Copyright 2013-2024 The OpenZipkin Authors * * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except * in compliance with the License. You may obtain a copy of the License at @@ -17,6 +17,7 @@ import brave.propagation.TraceContext; import brave.propagation.TraceContextOrSamplingFlags; import java.util.Arrays; +import java.util.concurrent.locks.ReentrantLock; /** * Holds extended state in {@link TraceContext#extra()} or {@link TraceContextOrSamplingFlags#extra()}. @@ -39,7 +40,7 @@ public abstract class Extra, F extends ExtraFactory> * Updates like {@link #tryToClaim(long, long)} lock on this object, as should any non-atomic * {@link #state} updates. */ - protected final Object lock = new Object(); + protected final ReentrantLock lock = new ReentrantLock(); /** * Lock on {@link #lock} when comparing existing values for a state update that happens after * {@link #mergeStateKeepingOursOnConflict(Extra)}. @@ -74,13 +75,16 @@ protected Extra(F factory) { /** Fields are extracted before a context is created. We need to lazy set the context */ final boolean tryToClaim(long traceId, long spanId) { - synchronized (lock) { + lock.lock(); + try { if (this.traceId == 0L) { this.traceId = traceId; this.spanId = spanId; return true; } return this.traceId == traceId && this.spanId == spanId; + } finally { + lock.unlock(); } } diff --git a/brave/src/main/java/brave/internal/extra/MapExtra.java b/brave/src/main/java/brave/internal/extra/MapExtra.java index d1659c1cb0..6c2fccac2b 100644 --- a/brave/src/main/java/brave/internal/extra/MapExtra.java +++ b/brave/src/main/java/brave/internal/extra/MapExtra.java @@ -92,7 +92,8 @@ protected boolean put(K key, @Nullable V value) { return false; } - synchronized (lock) { + lock.lock(); + try { Object[] prior = state(); // double-check lost race in dynamic case @@ -105,6 +106,8 @@ protected boolean put(K key, @Nullable V value) { newState[i + 1] = value; this.state = newState; return true; + } finally { + lock.unlock(); } }