Synchronize Baggage keyValues #4327

markushi · 2025-04-11T07:52:16Z

📜 Description

A Baggage can be mutable, but we don't synchronize it's internal keyValues, thus if e.g. Baggage.setTraceId() and Baggage.toHeaderString() are called simultaneously, a ConcurrentModification may be thrown.

SpanContext seems to be holding and creating reference copies of Baggage objects, so multiple spans happening at the same time are suspect to trigger this faulty behavior.

💡 Motivation and Context

Fixes #4321

💚 How did you test it?

📝 Checklist

I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
I updated the wizard if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

markushi · 2025-04-11T07:59:06Z

sentry/src/main/java/io/sentry/Baggage.java


-  @ApiStatus.Internal
-  public void set(final @NotNull String key, final @Nullable String value) {
-    set(key, value, false);


This was always called with force=false, so I just merged set(key, value, force) and set(key, value).

github-actions · 2025-04-11T08:05:21Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	410.46 ms	456.88 ms	46.42 ms
Size	1.58 MiB	2.08 MiB	505.64 KiB

Previous results on branch: markushi/fix/synchronize-mutable-baggages

Startup times

Revision	Plain	With Sentry	Diff
`351306b`	426.36 ms	460.65 ms	34.29 ms
`c529988`	428.26 ms	442.80 ms	14.54 ms
`9ae6786`	424.54 ms	443.76 ms	19.22 ms
`75efa54`	401.13 ms	456.45 ms	55.32 ms
`2aa945a`	429.48 ms	516.48 ms	87.00 ms

App size

Revision	Plain	With Sentry	Diff
`351306b`	1.58 MiB	2.08 MiB	505.74 KiB
`c529988`	1.58 MiB	2.08 MiB	505.64 KiB
`9ae6786`	1.58 MiB	2.08 MiB	505.69 KiB
`75efa54`	1.58 MiB	2.08 MiB	505.68 KiB
`2aa945a`	1.58 MiB	2.08 MiB	505.63 KiB

sentry/src/main/java/io/sentry/Baggage.java

adinauer · 2025-04-14T09:05:37Z

sentry/src/main/java/io/sentry/Baggage.java

-        if (value != null) {
-          final @NotNull String unknownKey = key.replaceFirst(SENTRY_BAGGAGE_PREFIX, "");
-          unknown.put(unknownKey, value);
+    synchronized (keyValues) {


We should use AutoClosableReentrantLock over synchronized as there was problems with virtual threads and synchronized before.

adinauer · 2025-04-14T09:07:04Z

sentry/src/main/java/io/sentry/Baggage.java


-    final Set<String> keys = new TreeSet<>(keyValues.keySet());
+    final Set<String> keys;
+    synchronized (keyValues) {


See below, replace with AutoClosableReentrantLock.

adinauer · 2025-04-14T09:10:39Z

sentry/src/main/java/io/sentry/Baggage.java

+      final boolean shouldFreeze,
      final @NotNull ILogger logger) {
-    this.keyValues = keyValues;
+    // TODO should we deep-copy the keyValues here?


No, we sometimes want to write to a shared baggage.

adinauer · 2025-04-14T09:11:08Z

sentry/src/main/java/io/sentry/Baggage.java

      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


There are cases where we forcefully overwrite the sample rate, even if mutable = false.

adinauer · 2025-04-14T09:13:55Z

sentry/src/main/java/io/sentry/Baggage.java

-    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);


Instead of wrapping here, which will cause problems with virtual threads again due to using synchronized internally, could we revert this line and instead always use ConcurrentHashMap in place of Map in this class?

adinauer · 2025-04-14T09:14:06Z

sentry/src/main/java/io/sentry/Baggage.java

-  final @NotNull Map<String, String> keyValues;
-  @Nullable Double sampleRate;
-  @Nullable Double sampleRand;
+  private final @NotNull Map<String, String> keyValues;


Suggested change

private final @NotNull Map<String, String> keyValues;

private final @NotNull ConcurrentHashMap<String, String> keyValues;

we should also change this in all locations of this file, where we have Map as a param or create a new HashMap. e.g. ctors and fromHeader

adinauer · 2025-04-14T09:16:00Z

sentry/src/main/java/io/sentry/SpanContext.java

    if (copiedUnknown != null) {
      this.unknown = copiedUnknown;
    }
+    // TODO should this be a deep copy instead?


No, we need to share entries.

adinauer

Did not mean to block this PR

adinauer

.

adinauer

LGTM, thanks for fixing!

Synchronize Baggage keyValues

46b17cb

markushi requested review from adinauer, lcian, romtsn and stefanosiano as code owners April 11, 2025 07:52

Update Changelog

9a5e29c

markushi commented Apr 11, 2025

View reviewed changes

stefanosiano approved these changes Apr 11, 2025

View reviewed changes

Add todo for SpanContext copy ctor

1eb8f8b

lcian reviewed Apr 11, 2025

View reviewed changes

sentry/src/main/java/io/sentry/Baggage.java Show resolved Hide resolved

Synchronize iterators

f610184

lcian approved these changes Apr 14, 2025

View reviewed changes

adinauer requested changes Apr 14, 2025

View reviewed changes

adinauer reviewed Apr 14, 2025

View reviewed changes

adinauer self-requested a review April 14, 2025 09:17

adinauer reviewed Apr 14, 2025

View reviewed changes

Use AutoClosableReentrantLock in favor of synchronized

ead5da3

adinauer approved these changes Apr 14, 2025

View reviewed changes

markushi added 2 commits April 14, 2025 13:43

Ensure null values work

3cc7514

Merge branch 'main' into markushi/fix/synchronize-mutable-baggages

cf9ee6c

markushi enabled auto-merge (squash) April 14, 2025 12:06

markushi merged commit 34b5305 into main Apr 14, 2025
35 checks passed

markushi deleted the markushi/fix/synchronize-mutable-baggages branch April 14, 2025 12:29

	private final @NotNull Map<String, String> keyValues;
	private final @NotNull ConcurrentHashMap<String, String> keyValues;

Uh oh!

Synchronize Baggage keyValues #4327

Synchronize Baggage keyValues #4327

Uh oh!

Conversation

markushi commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Previous results on branch: markushi/fix/synchronize-mutable-baggages

Startup times

App size

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinauer left a comment

Choose a reason for hiding this comment

Uh oh!

adinauer left a comment

Choose a reason for hiding this comment

Uh oh!

adinauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

markushi commented Apr 11, 2025 •

edited

Loading

github-actions bot commented Apr 11, 2025 •

edited

Loading