Fix double read of shared atomic in reactor instrumentation #1788
Conversation
| @@ -42,7 +42,7 @@ public TracingSubscriber(final AgentSpan upstreamSpan, final CoreSubscriber<T> d | |||
| if (downstreamScope != null) { | |||
| downstreamScope.setAsyncPropagation(true); | |||
| // create a hard reference to the trace that we don't want reported until we are done | |||
| continuation.set(activeScope().capture()); | |||
| continuation.set(downstreamScope.capture()); | |||
There was a problem hiding this comment.
This is just cleanup, of using the thing we actually read 4 lines above.
|
Makes my head hurt just trying to think about a test case. Completely up to the Reactor runtime to schedule things, and I don't know how I could coordinate things without getting to know more than I want about it 😉 |
|
Do you think we need a nondeterministic property based test which fires lots of requests at a reactor based application and verifies that the number of traces produced is the same as the number of requests? Would that have a better chance of verifying the issue exists? |
|
I think it would be good adding some property based tests for these types of asynchronous flows. |
|
Yes, that's a nice catch. I'm wondering if we could do some static analysis. This is the equivalent of calling Reference.get() testing non-null and then calling Reference.get() again. I suspect there's probably a case where someone wants to do something like that, but I think more often it would be an oversight. |
Occasional NPEs when things race.