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.

Sign up for GitHub

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

Jump to bottom

reactor: Always retry waitpid #2531

Merged
merged 1 commit into from
Nov 9, 2024
Merged

reactor: Always retry waitpid #2531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/seastar/core/reactor.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,7 @@ private:
void add_timer(timer<manual_clock>*) noexcept;
bool queue_timer(timer<manual_clock>*) noexcept;
void del_timer(timer<manual_clock>*) noexcept;
future<int> do_waitpid(pid_t pid);

future<> run_exit_tasks();
void stop();
Expand Down
62 changes: 30 additions & 32 deletions src/core/reactor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2075,6 +2075,32 @@ static auto next_waitpid_timeout(std::chrono::milliseconds this_timeout) {

#endif

future<int> reactor::do_waitpid(pid_t pid) {
return do_with(int{}, std::chrono::milliseconds(0), [pid, this](int& wstatus,
Copy link
Member

Choose a reason for hiding this comment

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

Please use coroutines in new code.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you're just moving old code around. ok.

Copy link
Member

Choose a reason for hiding this comment

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

#2532

std::chrono::milliseconds& wait_timeout) {
return repeat_until_value([this,
pid,
&wstatus,
&wait_timeout] {
return _thread_pool->submit<syscall_result<pid_t>>([pid, &wstatus] {
return wrap_syscall<pid_t>(::waitpid(pid, &wstatus, WNOHANG));
}).then([&wstatus, &wait_timeout](syscall_result<pid_t> ret) mutable {
if (ret.result == 0) {
wait_timeout = next_waitpid_timeout(wait_timeout);
return ::seastar::sleep(wait_timeout).then([] {
return make_ready_future<std::optional<int>>();
});
} else if (ret.result > 0) {
return make_ready_future<std::optional<int>>(wstatus);
} else {
ret.throw_if_error();
return make_ready_future<std::optional<int>>(-1);
}
});
});
});
}

future<int> reactor::waitpid(pid_t pid) {
return _thread_pool->submit<syscall_result<int>>([pid] {
return wrap_syscall<int>(syscall(__NR_pidfd_open, pid, O_NONBLOCK));
Expand All @@ -2083,39 +2109,11 @@ future<int> reactor::waitpid(pid_t pid) {
// pidfd_open() was introduced in linux 5.3, so the pidfd.error could be ENOSYS on
// older kernels. But it could be other error like EMFILE or ENFILE. anyway, we
// should always waitpid().
return do_with(int{}, std::chrono::milliseconds(0), [pid, this](int& wstatus,
std::chrono::milliseconds& wait_timeout) {
return repeat_until_value([this,
pid,
&wstatus,
&wait_timeout] {
return _thread_pool->submit<syscall_result<pid_t>>([pid, &wstatus] {
return wrap_syscall<pid_t>(::waitpid(pid, &wstatus, WNOHANG));
}).then([&wstatus, &wait_timeout] (syscall_result<pid_t> ret) mutable {
if (ret.result == 0) {
wait_timeout = next_waitpid_timeout(wait_timeout);
return ::seastar::sleep(wait_timeout).then([] {
return make_ready_future<std::optional<int>>();
});
} else if (ret.result > 0) {
return make_ready_future<std::optional<int>>(wstatus);
} else {
ret.throw_if_error();
return make_ready_future<std::optional<int>>(-1);
}
});
});
});
return do_waitpid(pid);
} else {
return do_with(pollable_fd(file_desc::from_fd(pidfd.result)), int{}, [pid, this](auto& pidfd, int& wstatus) {
return pidfd.readable().then([pid, &wstatus, this] {
return _thread_pool->submit<syscall_result<pid_t>>([pid, &wstatus] {
return wrap_syscall<pid_t>(::waitpid(pid, &wstatus, WNOHANG));
Copy link
Member

Choose a reason for hiding this comment

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

Presumably, using _thread_pool here (and above) is wrong, since WNOHANG means the kernel won't block.

});
}).then([&wstatus] (syscall_result<pid_t> ret) {
ret.throw_if_error();
assert(ret.result > 0);
return make_ready_future<int>(wstatus);
return do_with(pollable_fd(file_desc::from_fd(pidfd.result)), [pid, this](auto& pidfd) {
return pidfd.readable().then([pid, this] {
return do_waitpid(pid);
});
});
}
Expand Down