Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: revert the change from yield_now to sleep #964

Merged
merged 2 commits into from
May 31, 2024
Merged

Conversation

carneiro-cw
Copy link
Contributor

No description provided.

@carneiro-cw carneiro-cw requested a review from a team as a code owner May 31, 2024 15:31
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

2, because the PR involves a straightforward change from using sleep to yield_now in an asynchronous context. The change is localized and affects only one specific behavior in the code.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: Replacing sleep with yield_now might not ensure the intended delay. yield_now simply yields execution to the scheduler, potentially allowing other tasks to run, but it does not pause execution for a specific duration like sleep does. This could lead to issues if the delay was critical for functionality or performance.

🔒 Security concerns

No

Code feedback:
relevant filesrc/bin/importer_online.rs
suggestion      

Consider using an asynchronous delay mechanism that ensures a minimum wait time, such as sleep, if the delay is critical for the logic. Replacing sleep with yield_now might lead to unexpected behavior since yield_now does not guarantee any delay. [important]

relevant lineyield_now().await;

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible issue
Replace yield_now().await with sleep(INTERVAL_CATCH_UP).await to ensure a consistent delay

Replacing yield_now().await with sleep(INTERVAL_CATCH_UP).await might not be equivalent in
terms of functionality. yield_now() only yields the current task, allowing others to run,
which might not provide a sufficient delay if the system is under low load. This could
lead to a tight loop situation. Consider reintroducing a sleep with a defined interval to
ensure there is a delay.

src/bin/importer_online.rs [299]

-yield_now().await;
+sleep(INTERVAL_CATCH_UP).await;
 
Suggestion importance[1-10]: 9

Why: The suggestion correctly identifies a potential issue where yield_now().await may not provide a sufficient delay, especially under low system load. Reintroducing sleep(INTERVAL_CATCH_UP).await ensures a consistent delay, preventing a tight loop situation.

9

@carneiro-cw carneiro-cw enabled auto-merge (squash) May 31, 2024 15:38
@carneiro-cw carneiro-cw merged commit 82f40f9 into main May 31, 2024
32 checks passed
@carneiro-cw carneiro-cw deleted the yield_now branch May 31, 2024 15:57
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.

1 participant