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

Coroutinize waitpid #2532

Closed
wants to merge 2 commits into from
Closed

Conversation

bhalevy
Copy link
Member

@bhalevy bhalevy commented Nov 10, 2024

Commit fb6c969 simplified
waitpid by always retrying, even if pidfd_open failed.

Coroutinizing this path (and folding do_waitpid back into
its only caller: waitpid) simplifies the code even further
and consolidate the different do_with allocations into a single
coroutine frame.

}

std::chrono::milliseconds wait_timeout(0);
for (;;) {
Copy link
Member

Choose a reason for hiding this comment

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

Please don't code things as infinite loops, it means the reader has to reverse-engineer the termination condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in ee292c4

@bhalevy bhalevy force-pushed the coroutinize-waitpid branch 2 times, most recently from b0e2e4f to ee292c4 Compare November 10, 2024 12:30
@bhalevy
Copy link
Member Author

bhalevy commented Nov 10, 2024

In ee292c4:

  • defined do_waitpid as coroutine lambda to make loop explicit about the retryable case when the result is not ready yet
  • overall, the logic is clearer now

Commit fb6c969 simplified
waitpid by always retrying, even if pidfd_open failed.

Coroutinizing this path (and folding do_waitpid back into
its only caller: waitpid) simplifies the code even further
and consolidate the different do_with allocations into a single
coroutine frame.

Signed-off-by: Benny Halevy <[email protected]>
No need to define them as static variables.

Signed-off-by: Benny Halevy <[email protected]>
@bhalevy
Copy link
Member Author

bhalevy commented Nov 10, 2024

  • fixed spelling

@bhalevy
Copy link
Member Author

bhalevy commented Nov 13, 2024

@scylladb/seastar-maint please review / consider merging

@xemul xemul closed this in cd03667 Nov 14, 2024
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.

3 participants