Fixes race condition with toSpan and flush

[andylintner]

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]

Ping @anuraaga

On Tue, 7 Sep 2021, 23:43 Andy Lintner, @.***> wrote:

Fixes #1295 https://github.com/openzipkin/brave/issues/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:

https://github.com/openzipkin/brave/pull/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 https://github.com/openzipkin/brave/pull/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]

@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]

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

[jcchavezs]

@andylintner test is green.

[nkonev]

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

[jcchavezs]

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 https://github.com/openzipkin/brave/pull/1306#issuecomment-1140462989, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXOYAR5RHHNTM5MVX7Z66LVMN6SLANCNFSM5DTH2BKA . You are receiving this because you commented.Message ID: @.***>

[nkonev]

Any updates ?

[jcchavezs]

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

[jcchavezs]

Running this PR to see how the test fails without the fix https://github.com/openzipkin/brave/pull/1336

[jcchavezs]

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 https://github.com/openzipkin/brave/pull/1336

[frosiere]

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

[jcchavezs]

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

[codefromthecrypt]

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!