Skip to content

Commit

Permalink
Fix VirtualThread pinning with RealSpan and zipkin2.reporter by switc…
Browse files Browse the repository at this point in the history
…hing from synchronized to ReentrantLock

Signed-off-by: Andriy Redko <[email protected]>
  • Loading branch information
reta committed Mar 31, 2024
1 parent 69e30f3 commit 8c91589
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 9 deletions.
24 changes: 20 additions & 4 deletions brave/src/main/java/brave/RealSpan.java
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down
7 changes: 6 additions & 1 deletion brave/src/main/java/brave/internal/Platform.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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()}. */
Expand All @@ -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;
}
Expand Down
10 changes: 7 additions & 3 deletions brave/src/main/java/brave/internal/extra/Extra.java
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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()}.
Expand All @@ -39,7 +40,7 @@ public abstract class Extra<E extends Extra<E, F>, F extends ExtraFactory<E, F>>
* 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)}.
Expand Down Expand Up @@ -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();
}
}

Expand Down
5 changes: 4 additions & 1 deletion brave/src/main/java/brave/internal/extra/MapExtra.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -105,6 +106,8 @@ protected boolean put(K key, @Nullable V value) {
newState[i + 1] = value;
this.state = newState;
return true;
} finally {
lock.unlock();
}
}

Expand Down

0 comments on commit 8c91589

Please sign in to comment.