Skip to content

fix rpcz root client span lifetime (#3420) - #3421

Open
lh2debug-2 wants to merge 1 commit into
apache:masterfrom
lh2debug-2:fix-rpcz-root-client-span-lifetime
Open

fix rpcz root client span lifetime (#3420)#3421
lh2debug-2 wants to merge 1 commit into
apache:masterfrom
lh2debug-2:fix-rpcz-root-client-span-lifetime

Conversation

@lh2debug-2

Copy link
Copy Markdown

Fixes #3420

Summary

This change keeps the current RPC span alive from Controller until the RPC finishes, SubmitSpan runs, or the Controller is reset. This lets root client spans without a local parent be submitted to rpcz instead of being destroyed after the caller-side temporary shared_ptr goes out of scope.

Child client spans remain linked to their parent through weak local-parent references and parent-owned client lists, so they are still serialized under their parent without introducing shared_ptr cycles.

Testing

  • Built brpc successfully with CMake in /tmp/brpc-community-build-rpcz.
  • Ran a small local demo test verifying that a root client span remains alive after the caller-side shared_ptr is released while Controller still owns the span.

Note: the local CMake build used a temporary include-only workaround for an unrelated UBShm timer_mgr.cpp atomic_fetch_add/sub compile issue on GCC 8; no UBShm source change is included in this PR.

@wwbmmm

wwbmmm commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

There is a scenario like this:

  1. Server receive a request and create a local parent span
  2. During processing this request, a client start a new async call, create a child span
  3. Server send the response before the client return, the local parent span submit, and also submit the child span
  4. The client's response return, because the client span's parent is now expired, so the client will submit the span, too.
  5. Consequently, the client span is submitted twice.

Is that right?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes missing rpcz records for root client spans by ensuring the current span stays alive via Controller ownership until the RPC completes, SubmitSpan() runs, or the Controller is reset—preventing premature destruction when the caller-side temporary shared_ptr goes out of scope.

Changes:

  • Keep the active span alive by changing Controller::_span from std::weak_ptr<Span> to std::shared_ptr<Span>.
  • Update Controller span-access sites to use the strong pointer and preserve submission/reset behavior.
  • Clarify span submission comments to reflect that server spans and root client spans (no local parent) are submitted independently.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/brpc/span.cpp Updates comments to reflect root client span submission behavior (no local parent).
src/brpc/controller.h Changes _span from weak_ptr to shared_ptr to extend span lifetime through RPC completion/reset.
src/brpc/controller.cpp Updates span access patterns to use strong ownership and keep submission/reset flow consistent.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/brpc/controller.h
// NOTE: align and group fields to make Controller as compact as possible.

std::weak_ptr<Span> _span;
std::shared_ptr<Span> _span;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@lh2debug-2
lh2debug-2 force-pushed the fix-rpcz-root-client-span-lifetime branch from dfc0e1b to d6aad1a Compare August 3, 2026 05:36
@lh2debug-2

Copy link
Copy Markdown
Author

There is a scenario like this:

  1. Server receive a request and create a local parent span
  2. During processing this request, a client start a new async call, create a child span
  3. Server send the response before the client return, the local parent span submit, and also submit the child span
  4. The client's response return, because the client span's parent is now expired, so the client will submit the span, too.
  5. Consequently, the client span is submitted twice.

Is that right?

Thanks for pointing this out.

For the exact case where the parent span is submitted while the async child span is still active, the child is not serialized under the parent, because SpanDB::Index() only collects inactive child spans. Later, when the child finishes and its local parent has expired, it is submitted as a standalone client span. So that exact case does not duplicate the child.

The real duplicate risk is a nearby race: the child finishes first, the parent dump serializes it into parent.client_spans, then the child later submits itself after the parent expires.

I fixed this by making rpcz collection idempotent. Each Span now has a submitted flag. Both paths must claim it before writing:

  • standalone submit: Span::Submit()
  • nested child serialization: SpanDB::Index()
Case Result
Root server span submitted once as root
Root client span submitted once as root
Child span, parent alive not standalone submitted
Child ended, parent dumps first nested under parent; later standalone submit skipped
Parent dumps while child active not nested; later submitted standalone

Performance impact should be negligible. The flag is butil::atomic<bool> with relaxed ordering, sizeof(Span) does not increase, and the CAS is only on rpcz submit/dump paths. In the duplicate case it also reduces LevelDB writes by avoiding the extra standalone child record.

Keep the current RPC span alive from Controller until the RPC finishes,
SubmitSpan runs, or the Controller is reset. This lets root client spans
without a local parent be submitted to rpcz instead of being destroyed
after the caller-side temporary shared_ptr goes out of scope.

Child client spans remain linked to their parent through weak local-parent
references and parent-owned client lists, so they are still serialized
under their parent without introducing shared_ptr cycles.
@lh2debug-2
lh2debug-2 force-pushed the fix-rpcz-root-client-span-lifetime branch from d6aad1a to 84008ca Compare August 3, 2026 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rpcz does not record root client spans when Controller only keeps a weak_ptr to Span

3 participants