Skip to content

impl(o11y): box futures in DiscoveryPoller to prevent stack overflow#5918

Open
haphungw wants to merge 4 commits into
googleapis:mainfrom
haphungw:impl-o11y-box-discovery-poller-futures
Open

impl(o11y): box futures in DiscoveryPoller to prevent stack overflow#5918
haphungw wants to merge 4 commits into
googleapis:mainfrom
haphungw:impl-o11y-box-discovery-poller-futures

Conversation

@haphungw

@haphungw haphungw commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

When tracing is enabled, DiscoveryPoller is wrapped inside nesting decorators. Because it uses generic type parameters S (start future) and Q (query future), the compiler generates extremely deeply nested type definitions for every generic type combination.

At runtime, executing these deeply nested future types requires a lot of stack memory, resulting in a stack overflow in LRO-heavy tests (compute::run_compute_lro_errors).

Therefore, instead of keeping the futures as generic type parameters, we store them as boxed futures on the heap (Pin<Box<dyn Future + Send>>). This erases their concrete types and flattens their size on the stack.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request refactors DiscoveryPoller to use boxed futures (BoxedFuture) instead of generic parameters S and Q, simplifying the type signatures and trait implementations. The feedback suggests shadowing the query parameter instead of naming it query_clone to avoid confusion, as the variable is moved into the closure rather than cloned.

Comment thread src/lro/src/internal/discovery.rs Outdated
@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.90%. Comparing base (737706a) to head (b48f06e).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5918   +/-   ##
=======================================
  Coverage   97.90%   97.90%           
=======================================
  Files         234      234           
  Lines       59451    59458    +7     
=======================================
+ Hits        58204    58211    +7     
  Misses       1247     1247           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@haphungw haphungw marked this pull request as ready for review June 18, 2026 23:35
@haphungw haphungw requested a review from a team as a code owner June 18, 2026 23:35
}
}

type BoxedFuture<T> = std::pin::Pin<Box<dyn std::future::Future<Output = T> + Send + 'static>>;

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.

This should have a comment saying something about the tradeoffs we are making:

  • Instrumented LRO futures can get very large and cause stack overflows, so we need to box them.
  • The impact of moving these to the heap should be OK since polling is bound to network requests.

I'd also suggest having a Rust expert give you feedback as well, in case there are gotchas or alternatives we aren't considering.

Comment thread src/lro/src/internal/discovery.rs Outdated
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.

2 participants