Fixes race condition with toSpan and flush #1306

andylintner · 2021-09-07T21:42:57Z

Fixes #1295
Previous to this fix, a call to Tracer::toSpan concurrent with a call
to flush the span from pendingSpans could result in an assertion error.

We now only fetch the pendingSpan once for a single call to toSpan.

jcchavezs · 2021-09-08T11:15:08Z

Ping @anuraaga

…

On Tue, 7 Sep 2021, 23:43 Andy Lintner, ***@***.***> wrote: Fixes #1295 <#1295> Previous to this fix, a call to Tracer::toSpan concurrent with a call to flush the span from pendingSpans could result in an assertion error. We now only fetch the pendingSpan once for a single call to toSpan. ------------------------------ You can view, comment on, or merge this pull request online at: #1306 Commit Summary - Fixes race condition with toSpan and flush File Changes - *M* brave/src/main/java/brave/Tracer.java <https://github.com/openzipkin/brave/pull/1306/files#diff-8f5726dae04f25d1999a6091f638b8faf320502f01bc54d81e83124e917af9c3> (20) - *M* brave/src/test/java/brave/TracerTest.java <https://github.com/openzipkin/brave/pull/1306/files#diff-849a3d4ea020f209a97f36cbc19dd915e6e9cdd77c29d82249a3d68c5b3e0651> (22) Patch Links: - https://github.com/openzipkin/brave/pull/1306.patch - https://github.com/openzipkin/brave/pull/1306.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1306>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXOYASXL6JNT6VEXYSBZO3UA2BO7ANCNFSM5DTH2BKA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

andylintner · 2021-09-08T14:19:32Z

@anuraaga - I believe the test failure from the automation is just an inconsistent test. The same passes locally. Let me know if you think otherwise and I can dig further.

jcchavezs · 2021-09-09T19:49:46Z

@jeqo @jorgheymans any of you can give a check on this?

jcchavezs · 2021-09-09T20:17:11Z

@andylintner test is green.

jeqo

Overall seems like a sensible change, avoiding to get parent span twice to avoid race condition.

If we can make sure this is the only execution path that is affected by this issue, it looks good to me.

I wonder if instead of having an overloaded version of _toSpan, we could switch into the new interface TraceContext context, PendingSpan and move PendingSpan pendingSpan = pendingSpans.getOrCreate(parent, context, false); into the other usages of _toSpan. wdyt?

jeqo · 2021-09-10T19:58:14Z

brave/src/main/java/brave/Tracer.java

+  PendingSpan getPendingSpan(TraceContext context) {
+    return pendingSpans.get(context);
  }


could this one line method be replaced by a single call to pendingSpans.get(context)

jeqo · 2021-09-10T20:08:05Z

brave/src/main/java/brave/Tracer.java

-    TraceContext pendingContext = swapForPendingContext(context);
-    if (pendingContext != null) return _toSpan(parent, pendingContext);
+    // Re-use a pending span if present: This ensures reference consistency on Span.context()
+    PendingSpan pendingSpan = getPendingSpan(context);


Suggested change

PendingSpan pendingSpan = getPendingSpan(context);

PendingSpan pendingSpan = pendingSpans.get(context);

jeqo · 2021-09-10T20:11:24Z

brave/src/main/java/brave/Tracer.java

+    // Re-use a pending span if present: This ensures reference consistency on Span.context()
+    PendingSpan pendingSpan = getPendingSpan(context);
+    if (pendingSpan != null) {
+      if (isNoop(context)) return new NoopSpan(context);


Should this be moved into the _toSpan method?

It could, but it would mean either a redundant check with the call from the overloaded _toSpan, or eliminating the shortcut taken there that avoids unnecessary pendingSpan lookup.

nkonev · 2022-05-29T14:39:22Z

Hi @jeqo, @jcchavezs
I ran into this issue.
Can this PR be merged ?

jcchavezs · 2022-05-29T16:09:13Z

Ping @llinder søn. 29. mai 2022, 16:39 skrev Nikita Konev ***@***.***>:

…

Hi @jeqo <https://github.com/jeqo> I ran into this issue. Can this PR be merged ? — Reply to this email directly, view it on GitHub <#1306 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXOYAR5RHHNTM5MVX7Z66LVMN6SLANCNFSM5DTH2BKA> . You are receiving this because you commented.Message ID: ***@***.***>

nkonev · 2022-06-06T14:17:46Z

Any updates ?

jcchavezs · 2022-06-07T08:47:00Z

brave/src/main/java/brave/Tracer.java

@@ -192,12 +192,6 @@ public final Span joinSpan(TraceContext context) {
    }
  }

-  /** Returns an equivalent context if exists in the pending map */
-  TraceContext swapForPendingContext(TraceContext context) {


I wonder why this was removed.

jcchavezs · 2022-06-09T18:34:58Z

I need to get some time to try the test and show it fails. That will happen over the weekend.

brave/src/test/java/brave/TracerTest.java

jcchavezs · 2022-06-30T08:18:03Z

brave/src/main/java/brave/Tracer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2013-2020 The OpenZipkin Authors
+ * Copyright 2013-2021 The OpenZipkin Authors


jcchavezs · 2022-06-30T08:18:37Z

Running this PR to see how the test fails without the fix #1336

jcchavezs · 2022-06-30T09:25:30Z

Hi @andylintner unfortunately the unit test does not fail when the fix is not in place, could you please rework it so we show the failure? see #1336

frosiere · 2023-07-13T08:07:52Z

Hello, any news on this issue? We are facing the same issue. Any solution or workaround is more than welcome. Thanks.

jcchavezs · 2023-07-13T08:11:17Z

We need a test showing the issue otherwise we cannot merge the fix.

Fixes openzipkin#1295 Previous to this fix, a call to Tracer::toSpan concurrent with a call to flush the span from pendingSpans could result in an assertion error. We now only fetch the pendingSpan once for a single call to toSpan.

Signed-off-by: Adrian Cole <adrian@tetrate.io>

codefromthecrypt

I'm ok with these changes. Thanks for the help.

codefromthecrypt · 2023-12-15T12:29:04Z

according to #1336, this test passes before or after this change. I'll merge the latter, as the test needs to be improved in order to show what's wrong. When ready please raise a new PR!

Signed-off-by: Adrian Cole <adrian@tetrate.io> Co-authored-by: Adrian Cole <adrian@tetrate.io>

andylintner mentioned this pull request Sep 8, 2021

Parent can only be null in a local root spring-cloud/spring-cloud-sleuth#1814

Closed

jeqo reviewed Sep 10, 2021

View reviewed changes

jcchavezs reviewed Jun 7, 2022

View reviewed changes

jcchavezs added a commit that referenced this pull request Jun 30, 2022

chore: test to prove #1306

370f4b4

jcchavezs added a commit that referenced this pull request Jun 30, 2022

chore: test to prove #1306

2c31990

jcchavezs reviewed Jun 30, 2022

View reviewed changes

brave/src/test/java/brave/TracerTest.java Show resolved Hide resolved

jcchavezs reviewed Jun 30, 2022

View reviewed changes

alintnersfdc and others added 3 commits December 15, 2023 18:53

Fixes race condition with toSpan and flush

3536934

Fixes openzipkin#1295 Previous to this fix, a call to Tracer::toSpan concurrent with a call to flush the span from pendingSpans could result in an assertion error. We now only fetch the pendingSpan once for a single call to toSpan.

Address review feedback

c1b7446

licens

42a8705

Signed-off-by: Adrian Cole <adrian@tetrate.io>

codefromthecrypt force-pushed the concurrent-span-flush-fix branch from 2730fed to 42a8705 Compare December 15, 2023 11:54

codefromthecrypt approved these changes Dec 15, 2023

View reviewed changes

codefromthecrypt pushed a commit that referenced this pull request Dec 15, 2023

chore: test to prove #1306

c9b47b6

codefromthecrypt closed this Dec 15, 2023

codefromthecrypt mentioned this pull request Dec 15, 2023

chore: test to prove #1306 #1336

Merged

codefromthecrypt pushed a commit that referenced this pull request Dec 15, 2023

chore: test to prove #1306 (#1336)

5e15c20

Signed-off-by: Adrian Cole <adrian@tetrate.io> Co-authored-by: Adrian Cole <adrian@tetrate.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes race condition with toSpan and flush #1306

Fixes race condition with toSpan and flush #1306

andylintner commented Sep 7, 2021

jcchavezs commented Sep 8, 2021 via email

andylintner commented Sep 8, 2021

jcchavezs commented Sep 9, 2021

jcchavezs commented Sep 9, 2021

jeqo left a comment

jeqo Sep 10, 2021

jeqo Sep 10, 2021

jeqo Sep 10, 2021

andylintner Oct 12, 2021

nkonev commented May 29, 2022 •

edited

Loading

jcchavezs commented May 29, 2022 via email

nkonev commented Jun 6, 2022

jcchavezs Jun 7, 2022

jcchavezs commented Jun 9, 2022

jcchavezs Jun 30, 2022

jcchavezs commented Jun 30, 2022

jcchavezs commented Jun 30, 2022

frosiere commented Jul 13, 2023

jcchavezs commented Jul 13, 2023 •

edited

Loading

codefromthecrypt left a comment

codefromthecrypt commented Dec 15, 2023

	PendingSpan pendingSpan = getPendingSpan(context);
	PendingSpan pendingSpan = pendingSpans.get(context);

Fixes race condition with toSpan and flush #1306

Fixes race condition with toSpan and flush #1306

Conversation

andylintner commented Sep 7, 2021

jcchavezs commented Sep 8, 2021 via email

andylintner commented Sep 8, 2021

jcchavezs commented Sep 9, 2021

jcchavezs commented Sep 9, 2021

jeqo left a comment

Choose a reason for hiding this comment

jeqo Sep 10, 2021

Choose a reason for hiding this comment

jeqo Sep 10, 2021

Choose a reason for hiding this comment

jeqo Sep 10, 2021

Choose a reason for hiding this comment

andylintner Oct 12, 2021

Choose a reason for hiding this comment

nkonev commented May 29, 2022 • edited Loading

jcchavezs commented May 29, 2022 via email

nkonev commented Jun 6, 2022

jcchavezs Jun 7, 2022

Choose a reason for hiding this comment

jcchavezs commented Jun 9, 2022

jcchavezs Jun 30, 2022

Choose a reason for hiding this comment

jcchavezs commented Jun 30, 2022

jcchavezs commented Jun 30, 2022

frosiere commented Jul 13, 2023

jcchavezs commented Jul 13, 2023 • edited Loading

codefromthecrypt left a comment

Choose a reason for hiding this comment

codefromthecrypt commented Dec 15, 2023

nkonev commented May 29, 2022 •

edited

Loading

jcchavezs commented Jul 13, 2023 •

edited

Loading