From 46b17cbb1ad0f87a2902624e9fc12cb436b07777 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 11 Apr 2025 09:51:29 +0200 Subject: [PATCH 1/6] Synchronize Baggage keyValues --- sentry/src/main/java/io/sentry/Baggage.java | 32 ++++++++++----------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/sentry/src/main/java/io/sentry/Baggage.java b/sentry/src/main/java/io/sentry/Baggage.java index 4a5a6ca71d5..7f56885f686 100644 --- a/sentry/src/main/java/io/sentry/Baggage.java +++ b/sentry/src/main/java/io/sentry/Baggage.java @@ -13,6 +13,7 @@ import java.text.DecimalFormatSymbols; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Locale; @@ -44,13 +45,13 @@ protected DecimalFormat initialValue() { private static final DecimalFormatterThreadLocal decimalFormatter = new DecimalFormatterThreadLocal(); - final @NotNull Map keyValues; - @Nullable Double sampleRate; - @Nullable Double sampleRand; + private final @NotNull Map keyValues; + private @Nullable Double sampleRate; + private @Nullable Double sampleRand; - final @Nullable String thirdPartyHeader; + private final @Nullable String thirdPartyHeader; private boolean mutable; - private boolean shouldFreeze; + private final boolean shouldFreeze; final @NotNull ILogger logger; @NotNull @@ -217,10 +218,12 @@ public Baggage( final @Nullable Double sampleRate, final @Nullable Double sampleRand, final @Nullable String thirdPartyHeader, - boolean isMutable, - boolean shouldFreeze, + final boolean isMutable, + final boolean shouldFreeze, final @NotNull ILogger logger) { - this.keyValues = keyValues; + // TODO should we deep-copy the keyValues here? + // if so we could optimize this by only synchronizing in case isMutable is true + this.keyValues = Collections.synchronizedMap(keyValues); this.sampleRate = sampleRate; this.sampleRand = sampleRand; this.logger = logger; @@ -440,20 +443,15 @@ public void setReplayId(final @Nullable String replayId) { set(DSCKeys.REPLAY_ID, replayId); } - @ApiStatus.Internal - public void set(final @NotNull String key, final @Nullable String value) { - set(key, value, false); - } - /** - * Sets / updates a value + * Sets / updates a value, but only if the baggage is still mutable. * * @param key key * @param value value to set - * @param force ignores mutability of this baggage and sets the value anyways */ - private void set(final @NotNull String key, final @Nullable String value, final boolean force) { - if (mutable || force) { + @ApiStatus.Internal + public void set(final @NotNull String key, final @Nullable String value) { + if (mutable) { this.keyValues.put(key, value); } } From 9a5e29c00864c49bdf0a8eff791770f2dd04c917 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 11 Apr 2025 09:53:07 +0200 Subject: [PATCH 2/6] Update Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 049b2a818da..3578c258dfc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - This ensures correct resource loading in environments like Spring Boot where the thread context classloader is used for resource loading. - Improve low memory breadcrumb capturing ([#4325](https://github.com/getsentry/sentry-java/pull/4325)) - Fix do not initialize SDK for Jetpack Compose Preview builds ([#4324](https://github.com/getsentry/sentry-java/pull/4324)) +- Fix Synchronize Baggage values ([#4327](https://github.com/getsentry/sentry-java/pull/4327)) ## 8.7.0 From 1eb8f8b639b88f0a650848801aa2c42d7d996632 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Fri, 11 Apr 2025 11:55:20 +0200 Subject: [PATCH 3/6] Add todo for SpanContext copy ctor --- sentry/src/main/java/io/sentry/SpanContext.java | 1 + 1 file changed, 1 insertion(+) diff --git a/sentry/src/main/java/io/sentry/SpanContext.java b/sentry/src/main/java/io/sentry/SpanContext.java index 2999ea4a2b8..183bda61005 100644 --- a/sentry/src/main/java/io/sentry/SpanContext.java +++ b/sentry/src/main/java/io/sentry/SpanContext.java @@ -125,6 +125,7 @@ public SpanContext(final @NotNull SpanContext spanContext) { if (copiedUnknown != null) { this.unknown = copiedUnknown; } + // TODO should this be a deep copy instead? this.baggage = spanContext.baggage; final Map copiedData = CollectionUtils.newConcurrentHashMap(spanContext.data); if (copiedData != null) { From f610184dca1b774174dff5b9be1ddc7999f20861 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Mon, 14 Apr 2025 08:19:05 +0200 Subject: [PATCH 4/6] Synchronize iterators --- sentry/src/main/java/io/sentry/Baggage.java | 22 ++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/sentry/src/main/java/io/sentry/Baggage.java b/sentry/src/main/java/io/sentry/Baggage.java index 7f56885f686..c8686a7ac21 100644 --- a/sentry/src/main/java/io/sentry/Baggage.java +++ b/sentry/src/main/java/io/sentry/Baggage.java @@ -263,7 +263,10 @@ public String getThirdPartyHeader() { separator = ","; } - final Set keys = new TreeSet<>(keyValues.keySet()); + final Set keys; + synchronized (keyValues) { + keys = new TreeSet<>(keyValues.keySet()); + } keys.add(DSCKeys.SAMPLE_RATE); keys.add(DSCKeys.SAMPLE_RAND); @@ -459,17 +462,18 @@ public void set(final @NotNull String key, final @Nullable String value) { @ApiStatus.Internal public @NotNull Map getUnknown() { final @NotNull Map unknown = new ConcurrentHashMap<>(); - for (Map.Entry keyValue : this.keyValues.entrySet()) { - final @NotNull String key = keyValue.getKey(); - final @Nullable String value = keyValue.getValue(); - if (!DSCKeys.ALL.contains(key)) { - if (value != null) { - final @NotNull String unknownKey = key.replaceFirst(SENTRY_BAGGAGE_PREFIX, ""); - unknown.put(unknownKey, value); + synchronized (keyValues) { + for (final Map.Entry keyValue : keyValues.entrySet()) { + final @NotNull String key = keyValue.getKey(); + final @Nullable String value = keyValue.getValue(); + if (!DSCKeys.ALL.contains(key)) { + if (value != null) { + final @NotNull String unknownKey = key.replaceFirst(SENTRY_BAGGAGE_PREFIX, ""); + unknown.put(unknownKey, value); + } } } } - return unknown; } From ead5da3bffdf251feb4766ef98dd7adb51347b09 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Mon, 14 Apr 2025 12:28:27 +0200 Subject: [PATCH 5/6] Use AutoClosableReentrantLock in favor of synchronized --- sentry/api/sentry.api | 2 +- sentry/src/main/java/io/sentry/Baggage.java | 24 ++++++++++--------- .../src/main/java/io/sentry/SpanContext.java | 1 - 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 067fa327ed4..670728fbaaa 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -35,7 +35,7 @@ public abstract interface class io/sentry/BackfillingEventProcessor : io/sentry/ public final class io/sentry/Baggage { public fun (Lio/sentry/Baggage;)V public fun (Lio/sentry/ILogger;)V - public fun (Ljava/util/Map;Ljava/lang/Double;Ljava/lang/Double;Ljava/lang/String;ZZLio/sentry/ILogger;)V + public fun (Ljava/util/concurrent/ConcurrentHashMap;Ljava/lang/Double;Ljava/lang/Double;Ljava/lang/String;ZZLio/sentry/ILogger;)V public fun forceSetSampleRate (Ljava/lang/Double;)V public fun freeze ()V public static fun fromEvent (Lio/sentry/SentryEvent;Lio/sentry/SentryOptions;)Lio/sentry/Baggage; diff --git a/sentry/src/main/java/io/sentry/Baggage.java b/sentry/src/main/java/io/sentry/Baggage.java index c8686a7ac21..238ce95fe27 100644 --- a/sentry/src/main/java/io/sentry/Baggage.java +++ b/sentry/src/main/java/io/sentry/Baggage.java @@ -4,6 +4,7 @@ import io.sentry.protocol.SentryId; import io.sentry.protocol.TransactionNameSource; +import io.sentry.util.AutoClosableReentrantLock; import io.sentry.util.SampleRateUtils; import io.sentry.util.StringUtils; import java.io.UnsupportedEncodingException; @@ -14,7 +15,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -45,7 +45,9 @@ protected DecimalFormat initialValue() { private static final DecimalFormatterThreadLocal decimalFormatter = new DecimalFormatterThreadLocal(); - private final @NotNull Map keyValues; + private final @NotNull ConcurrentHashMap keyValues; + private final @NotNull AutoClosableReentrantLock keyValuesLock = new AutoClosableReentrantLock(); + private @Nullable Double sampleRate; private @Nullable Double sampleRand; @@ -100,7 +102,9 @@ public static Baggage fromHeader( final @Nullable String headerValue, final boolean includeThirdPartyValues, final @NotNull ILogger logger) { - final @NotNull Map keyValues = new HashMap<>(); + + final @NotNull ConcurrentHashMap keyValues = new ConcurrentHashMap<>(); + final @NotNull List thirdPartyKeyValueStrings = new ArrayList<>(); boolean shouldFreeze = false; @@ -197,7 +201,7 @@ public static Baggage fromEvent( @ApiStatus.Internal public Baggage(final @NotNull ILogger logger) { - this(new HashMap<>(), null, null, null, true, false, logger); + this(new ConcurrentHashMap<>(), null, null, null, true, false, logger); } @ApiStatus.Internal @@ -214,16 +218,14 @@ public Baggage(final @NotNull Baggage baggage) { @ApiStatus.Internal public Baggage( - final @NotNull Map keyValues, + final @NotNull ConcurrentHashMap keyValues, final @Nullable Double sampleRate, final @Nullable Double sampleRand, final @Nullable String thirdPartyHeader, final boolean isMutable, final boolean shouldFreeze, final @NotNull ILogger logger) { - // TODO should we deep-copy the keyValues here? - // if so we could optimize this by only synchronizing in case isMutable is true - this.keyValues = Collections.synchronizedMap(keyValues); + this.keyValues = keyValues; this.sampleRate = sampleRate; this.sampleRand = sampleRand; this.logger = logger; @@ -264,8 +266,8 @@ public String getThirdPartyHeader() { } final Set keys; - synchronized (keyValues) { - keys = new TreeSet<>(keyValues.keySet()); + try (final @NotNull ISentryLifecycleToken ignored = keyValuesLock.acquire()) { + keys = new TreeSet<>(Collections.list(keyValues.keys())); } keys.add(DSCKeys.SAMPLE_RATE); keys.add(DSCKeys.SAMPLE_RAND); @@ -462,7 +464,7 @@ public void set(final @NotNull String key, final @Nullable String value) { @ApiStatus.Internal public @NotNull Map getUnknown() { final @NotNull Map unknown = new ConcurrentHashMap<>(); - synchronized (keyValues) { + try (final @NotNull ISentryLifecycleToken ignored = keyValuesLock.acquire()) { for (final Map.Entry keyValue : keyValues.entrySet()) { final @NotNull String key = keyValue.getKey(); final @Nullable String value = keyValue.getValue(); diff --git a/sentry/src/main/java/io/sentry/SpanContext.java b/sentry/src/main/java/io/sentry/SpanContext.java index 183bda61005..2999ea4a2b8 100644 --- a/sentry/src/main/java/io/sentry/SpanContext.java +++ b/sentry/src/main/java/io/sentry/SpanContext.java @@ -125,7 +125,6 @@ public SpanContext(final @NotNull SpanContext spanContext) { if (copiedUnknown != null) { this.unknown = copiedUnknown; } - // TODO should this be a deep copy instead? this.baggage = spanContext.baggage; final Map copiedData = CollectionUtils.newConcurrentHashMap(spanContext.data); if (copiedData != null) { From 3cc7514a4275aa1b9b330e3217cf87b2f46c0721 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Mon, 14 Apr 2025 13:43:32 +0200 Subject: [PATCH 6/6] Ensure null values work --- sentry/src/main/java/io/sentry/Baggage.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/sentry/src/main/java/io/sentry/Baggage.java b/sentry/src/main/java/io/sentry/Baggage.java index 238ce95fe27..52b7016e6d5 100644 --- a/sentry/src/main/java/io/sentry/Baggage.java +++ b/sentry/src/main/java/io/sentry/Baggage.java @@ -457,7 +457,11 @@ public void setReplayId(final @Nullable String replayId) { @ApiStatus.Internal public void set(final @NotNull String key, final @Nullable String value) { if (mutable) { - this.keyValues.put(key, value); + if (value == null) { + keyValues.remove(key); + } else { + keyValues.put(key, value); + } } }