perf(tunnel): reuse HTTP client in CustomTunnel health checks#1201
perf(tunnel): reuse HTTP client in CustomTunnel health checks#1201willamhou wants to merge 3 commits intonearai:stagingfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the performance of health checks within the Highlights
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request improves performance by reusing the reqwest::Client for health checks, which is a good optimization. I've found one area for improvement regarding error handling during client creation to ensure robustness and prevent a potential regression in behavior. My feedback is detailed in the comment below.
src/tunnel/custom.rs
Outdated
| http_client: reqwest::Client::builder() | ||
| .timeout(std::time::Duration::from_secs(5)) | ||
| .build() | ||
| .unwrap_or_else(|_| reqwest::Client::new()), |
There was a problem hiding this comment.
Using unwrap_or_else here could lead to unexpected behavior. If build() fails, it falls back to a default reqwest::Client that doesn't have the configured timeout. This is a regression, as the previous implementation always had a 5-second timeout, and could cause health checks to hang indefinitely.
A more robust solution would be to propagate the error from build(). This would involve changing CustomTunnel::new to return a Result<Self>:
// In src/tunnel/custom.rs
pub fn new(...) -> anyhow::Result<Self> {
Ok(Self {
// ...
http_client: reqwest::Client::builder()
.timeout(std::time::Duration::from_secs(5))
.build()?,
})
}This would also require updating the call site in src/tunnel/mod.rs to handle the Result with ?. This ensures that a failure to create the HTTP client is handled explicitly instead of silently creating a client with different behavior.
References
- Avoid fallbacks like
unwrap_or()orunwrap_or_else()that could cause the system to silently check the wrong logic or operate in an unexpected state. Prefer explicit error propagation or failure.
zmanian
left a comment
There was a problem hiding this comment.
Review: REQUEST CHANGES
Good idea, clean implementation -- reusing reqwest::Client avoids per-call TLS negotiation and connection pool setup. Constructor correctly changed to Result<Self>.
Blocking: missing test update
The stdout_drain_prevents_zombie test (~line 274) still calls CustomTunnel::new("yes".into(), None, None) without .unwrap(). Since the constructor now returns Result<Self>, this will fail to compile. 6 of 7 test callsites were updated, this one was missed. Trivial fix:
let tunnel = CustomTunnel::new("yes".into(), None, None).unwrap();CI concern: Only classify/scope checks ran -- no build/test. This would have caught the missing .unwrap().
Everything else looks good: timeout correctly moved to client builder, error handling uses .context(), no .unwrap() in production code. Once this one-liner is fixed and CI runs green, this is ready to approve.
Previously, `health_check()` created a new `reqwest::Client` on every call. Since health checks run periodically, this caused unnecessary TLS session setup and connection pool churn. Store a shared client on the struct instead, created once in the constructor with a 5s timeout. Fixes nearai#1039 Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
…back Address review feedback: `unwrap_or_else(|_| Client::new())` silently drops the configured timeout on builder failure. Change `new()` to return `Result<Self>` and propagate with `?` instead. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
CustomTunnel::new was changed to return Result<Self> but this test callsite was missed. Fixes compilation error. [skip-regression-check]
edf8bdb to
8f96f85
Compare
Summary
reqwest::ClientonCustomTunnelstruct instead of creating one perhealth_check()callFixes #1039
Test plan
cargo checkpassescargo test -- tunnel)health_with_unreachable_url_is_falsetest exercises the changed code path🤖 Generated with Claude Code