Use tokio sleep instead of blocking thread when using reqwest client
#689
Conversation
WalkthroughThe changes introduce a new abstraction, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpClient
participant SleepBackend
Client->>HttpClient: is_tokio()
Client->>SleepBackend: infer(is_tokio)
loop while waiting for task
Client->>SleepBackend: sleep(interval)
SleepBackend-->>Client: (sleeps using Tokio, Thread, or JS)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
paolobarbolini
force-pushed
the
sleep-without-thread
branch
from
f11ec55 to
1b32767
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils.rs (2)
36-43: Consider adding error handling for thread panics.The current implementation could hang indefinitely if the spawned thread panics before sending the signal. Consider adding timeout or panic handling.
Self::Thread => { let (sender, receiver) = futures_channel::oneshot::channel::<()>(); std::thread::spawn(move || { + let _guard = std::panic::catch_unwind(|| { std::thread::sleep(interval); + }); let _ = sender.send(()); }); let _ = receiver.await; }
45-60: Consider more robust error handling for JavaScript implementation.The multiple
unwrap()calls could cause panics in edge cases. Consider handling potential failures more gracefully, especially for the duration conversion which could overflow for very large durations.Self::Javascript => { use std::convert::TryInto; use wasm_bindgen_futures::JsFuture; + let timeout_ms: i32 = interval.as_millis().try_into() + .unwrap_or(i32::MAX); + JsFuture::from(web_sys::js_sys::Promise::new(&mut |yes, _| { - web_sys::window() - .unwrap() - .set_timeout_with_callback_and_timeout_and_arguments_0( - &yes, - interval.as_millis().try_into().unwrap(), - ) - .unwrap(); + if let Some(window) = web_sys::window() { + let _ = window.set_timeout_with_callback_and_timeout_and_arguments_0( + &yes, + timeout_ms, + ); + } })) .await - .unwrap(); + .unwrap_or_default(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Cargo.toml(2 hunks)src/client.rs(3 hunks)src/request.rs(1 hunks)src/reqwest.rs(1 hunks)src/utils.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/reqwest.rs
- src/request.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- src/client.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/utils.rs (3)
src/reqwest.rs (3)
is_tokio(116-118)new(24-51)new(148-155)src/request.rs (1)
is_tokio(105-107)meilisearch-test-macro/src/lib.rs (1)
meilisearch_test(14-201)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-tests
🔇 Additional comments (3)
src/utils.rs (3)
3-11: Well-structured enum with appropriate conditional compilation.The
SleepBackendenum design effectively abstracts over different sleep implementations based on target architecture and feature availability. The cfg conditions are logically structured to ensure the appropriate variant is available for each platform.
14-27: Inference logic correctly handles all platform and feature combinations.The method properly selects the appropriate sleep backend based on target architecture, feature availability, and runtime detection. The conditional compilation ensures only relevant code paths are included for each target platform.
70-101: Comprehensive test coverage for all backend variants.The tests effectively verify each sleep backend implementation with appropriate platform-specific conditional compilation. The timing assertions correctly validate that sleep duration is respected.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #689 +/- ##
==========================================
- Coverage 85.76% 85.67% -0.10%
==========================================
Files 19 19
Lines 5860 5883 +23
==========================================
+ Hits 5026 5040 +14
- Misses 834 843 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hello! Thank you for your contribution. Our current implementation definitely needs to be improved. Your SleepBackend abstraction is interesting, but why would someone ever want to use the thread-blocking sleep? I would be in favor of just using tokio's sleep for everything except on wasm and removing the abstraction.
AFAIK your implementation, except for the |
paolobarbolini
force-pushed
the
sleep-without-thread
branch
from
1b32767 to
6ef36ce
Compare
|
Thanks for the quick response! I still do not see why anyone would want to use the old thread-blocking sleep implementation. Tokio's sleep should always be best, so we can remove the old one. Also, that makes SleepBackend redundant. I think we just need an internal |
|
The below official code example would stop working because tokio sleep can only be used within a tokio runtime, so it's not compatible with the |
Pull Request
Related issue
None
What does this PR do?
This PR makes meilisearch-sdk use the much more efficient tokio sleep machinery when the
reqwestHTTP client is used to make requests (we assumereqwestmeans that the user is usingtokiobecause it only works withtokio) instead of spawing a new ad hoc blocking thread for each sleep request.PR checklist
Summary by CodeRabbit