Span::CreateClientSpan是否有线程安全问题

[fausturs]

Describe the bug (描述bug)

https://github.com/apache/brpc/blob/master/src/brpc/span.cpp#L133

在代码这个位置，如果是在多个bthread中调用的Span::CreateClientSpan，而他们属于同一个Span的child span。此时他们对brpc::Span::_next_client是存在竞态的。
即便认为这个指针的赋值是原子的，似乎也可能导致内存泄漏。

这里是有什么特殊的设计避免了竞态吗？求解答

To Reproduce (复现方法)

Expected behavior (期望行为)

Versions (各种版本)
OS:
Compiler:
brpc:
protobuf:

Additional context/screenshots (更多上下文/截图)

[fausturs]

补充一下，我跑的测试代码

brpc部分，就是随便加点输出信息。

搞一个server，类似这样，在处理request得时候提交两个子任务。 一个可能的输出是这样的。 可以看到，在时间轴上，两个线程分别设置了span->_next_client之后，再分别设置了parent->_next_client。这个时候容易知道链表中只有一个节点了（预期是两个）。此时内存泄漏了。  con从rpcz页面中也能看到，client span只有一个了。

如果同时写parent->_next_client，则可能写坏内存，导致出core。这个就比较难复现了。

[fausturs]

rpcz的截图补充

[wwbmmm]

是存在这个问题

[bigolcy]

上述问题可以通过使用cas进行链表插入来解决，当然brpc的span不止这一处问题，收到请求包后发起异步调用，svr span会被析构。收到异步调用回包后再发起rpc就可能core。done->Run()提前回包后再发起rpc也有类似的问题。
为了解决这个问题我们对svr span加入了引用计数机制。在合适的位置为当前的svr span引用计数+1，析构时-1。算是比较彻底的解决了这个问题。
在这些问题解决前，不建议在生产环境使用brpc的trace。

[fausturs]

上述问题可以通过使用cas进行链表插入来解决，当然brpc的span不止这一处问题，收到请求包后发起异步调用，svr span会被析构。收到异步调用回包后再发起rpc就可能core。done->Run()提前回包后再发起rpc也有类似的问题。 为了解决这个问题我们对svr span加入了引用计数机制。在合适的位置为当前的svr span引用计数+1，析构时-1。算是比较彻底的解决了这个问题。 在这些问题解决前，不建议在生产环境使用brpc的trace。

hello，想请教一下“收到异步调用回包后再发起rpc就可能core”，异步回调中发起rpc时，所在的bthread的thread local的变量中，应该不包含原来那个svr span的指针吧。因为这可能会是一个新的bthread，那么会是什么原因导致的core呢？

从源代码来看（https://github.com/apache/brpc/blob/master/src/brpc/controller.cpp#L718），创建新的bthread的时候，也没有使用BTHREAD_INHERIT_SPAN标志。brpc这个时候，可能不认为新的rpc请求是老的svr span的child span了。不知道我理解的是否有问题
@bigolcy

[bigolcy]

我们的代码与主干相距过远了，具体原因可能得看看现在的代码实现是否还有问题。https://github.com/apache/brpc/blob/master/src/brpc/span.cpp#L422 没记错的话，这里析构会把client span一起析构。异步发包后再访问client span就可能core了，跟是否有rpc应该关系不大。

[bigolcy]

done->Run()提前回包后再发起rpc，这个问题仍然存在的。@fausturs
本质问题是span的生命周期并不清晰。

[yanglimingcn]

上述问题可以通过使用cas进行链表插入来解决，当然brpc的span不止这一处问题，收到请求包后发起异步调用，svr span会被析构。收到异步调用回包后再发起rpc就可能core。done->Run()提前回包后再发起rpc也有类似的问题。 为了解决这个问题我们对svr span加入了引用计数机制。在合适的位置为当前的svr span引用计数+1，析构时-1。算是比较彻底的解决了这个问题。 在这些问题解决前，不建议在生产环境使用brpc的trace。

hello，想请教一下“收到异步调用回包后再发起rpc就可能core”，异步回调中发起rpc时，所在的bthread的thread local的变量中，应该不包含原来那个svr span的指针吧。因为这可能会是一个新的bthread，那么会是什么原因导致的core呢？

从源代码来看（https://github.com/apache/brpc/blob/master/src/brpc/controller.cpp#L718），创建新的bthread的时候，也没有使用BTHREAD_INHERIT_SPAN标志。brpc这个时候，可能不认为新的rpc请求是老的svr span的child span了。不知道我理解的是否有问题 @bigolcy

感觉你可以在父bthread里面创建多个span，然后作为argument传递给每个子bthread。

[yanglimingcn]

@wwbmmm 这个问题我最近想出一个方案来解决，想和你讨论一下：
现在启动bthread如果加上BTHREAD_INHERIT_SPAN标志，bthread会继承span，为了修复这个问题，简单的方法是在创建bthread的时候，如果有BTHREAD_INHERIT_SPAN这个标志，就执行一次Span::CreateClientSpan，创建一个client span，让这个client span成为m->local_storage.rpcz_parent_span。
这样做的
优势在于，创建span仍然是能够完全规避了线程竞争，所以对性能应该很友好。
缺点在于，会增加一些span的数量。
实现难度很低，但是也会有个问题，就是需要在bthread层面调用Span::CreateClientSpan，但是Span::CreateClientSpan是在brpc这个层面上，代码调用关系可能有些不合理。

[fausturs]

优势在于，创建span仍然是能够完全规避了线程竞争，所以对性能应该很友好。

这个修改没有想象中那么简单，至少还涉及了rpcz相关的代码。这里会出现多个trace id，span id一样的span对象。
那么存入leveldb时，可能要考虑是否合并到一起。
如果不合并则要考虑rpcz的展示需要做一些修改。

所以，加一个锁，或者一个lockfree的队列会来的更快。

另外还注意到TRACEPRINTF这个宏其实可能也有线程安全问题，最终其实是调用到Span::Annotate这个函数。
如果是多个线程同时写，则显然需要一个锁。

所以总的来说，除非trace部分整体重新设计一遍，否则，使用锁是一个看上去更清晰的方案。

[yanglimingcn]

优势在于，创建span仍然是能够完全规避了线程竞争，所以对性能应该很友好。

这个修改没有想象中那么简单，至少还涉及了rpcz相关的代码。这里会出现多个trace id，span id一样的span对象。 那么存入leveldb时，可能要考虑是否合并到一起。 如果不合并则要考虑rpcz的展示需要做一些修改。

所以，加一个锁，或者一个lockfree的队列会来的更快。

另外还注意到TRACEPRINTF这个宏其实可能也有线程安全问题，最终其实是调用到Span::Annotate这个函数。 如果是多个线程同时写，则显然需要一个锁。

所以总的来说，除非trace部分整体重新设计一遍，否则，使用锁是一个看上去更清晰的方案。

单线程调用内部多次调用createClientSpan，这个应该不会造成重复spanId吧，多线程调用Annotate相当于多线程并发处理一个请求？

[fausturs]

优势在于，创建span仍然是能够完全规避了线程竞争，所以对性能应该很友好。

这个修改没有想象中那么简单，至少还涉及了rpcz相关的代码。这里会出现多个trace id，span id一样的span对象。 那么存入leveldb时，可能要考虑是否合并到一起。 如果不合并则要考虑rpcz的展示需要做一些修改。
所以，加一个锁，或者一个lockfree的队列会来的更快。
另外还注意到TRACEPRINTF这个宏其实可能也有线程安全问题，最终其实是调用到Span::Annotate这个函数。 如果是多个线程同时写，则显然需要一个锁。
所以总的来说，除非trace部分整体重新设计一遍，否则，使用锁是一个看上去更清晰的方案。

单线程调用内部多次调用createClientSpan，这个应该不会造成重复spanId吧，多线程调用Annotate相当于多线程并发处理一个请求？

如果span id不相同，那么是不是会意味着逻辑上的一个span，在代码中实际上被处理成多个不同的span了？当然，处理成多个不同的span倒是也还蛮好，或许可以参考open telemetry，使用一个新的span类型，比如叫internal span。

关于TRACEPRINTF，我想表达的意思是，在现有的情况下，是对bthread::tls_bls.rpcz_parent_span这个对象进行写入，当多个线程同时写入的时候，会不会有线程安全问题？不过按照你描述的这种改法来看，修改后应该不存在这个问题了。
https://github.com/apache/brpc/blob/master/src/brpc/span.cpp#L298

[wwbmmm]

实现难度很低，但是也会有个问题，就是需要在bthread层面调用Span::CreateClientSpan，但是Span::CreateClientSpan是在brpc这个层面上，代码调用关系可能有些不合理。

这个可以让bthread开放一个set_create_span_func的方法，然后在brpc代码里将真正实现的方法设置进去

[yanglimingcn]

实现难度很低，但是也会有个问题，就是需要在bthread层面调用Span::CreateClientSpan，但是Span::CreateClientSpan是在brpc这个层面上，代码调用关系可能有些不合理。

这个可以让bthread开放一个set_create_span_func的方法，然后在brpc代码里将真正实现的方法设置进去

#2519