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

Issue 20257 - Add wait(Pid, Duration) for windows #7528

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

MoonlightSentinel
Copy link
Contributor

@MoonlightSentinel MoonlightSentinel commented Jun 12, 2020

wait(Pid, Duration) allows to wait until the process terminates
or the timeout expires.

It's currently windows-only because a decent posix implementation
is quite difficult and this allows us to sort out the API beforehand.

CC @andralex as this introduces a new symbol

@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 12, 2020

Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
20257 enhancement Implement waitTimeout() in std.process

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7528"

Copy link
Member

@CyberShadow CyberShadow left a comment

Choose a reason for hiding this comment

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

Good addition though unsure about more platform-specific functionality to Phobos. Would be good to ensure that it's possible to implement such things on top of Phobos functionality (i.e. using Pid.osHandle).

std/process.d Outdated Show resolved Hide resolved
std/process.d Outdated Show resolved Hide resolved
std/process.d Show resolved Hide resolved
@MoonlightSentinel
Copy link
Contributor Author

Good addition though unsure about more platform-specific functionality to Phobos.

Agreed, implementations for other platforms should be added in the future.

Would be good to ensure that it's possible to implement such things on top of Phobos functionality (i.e. using Pid.osHandle).

You mean a generic implementation for other platforms that doesn't depend on os-specific functions?

@CyberShadow
Copy link
Member

You mean a generic implementation for other platforms that doesn't depend on os-specific functions?

Not necessarily. I recall that there was some friction with doing some things which are partially doable with std.process. E.g., kill(Pid) would work perfectly fine for processes which are actually not children of the D program, however, there is no way to construct a Pid for an arbitrary process. I think we should try to avoid such situations when possible, and strive for interoperability between the D wrappers and the low-level C/OS API. In this case, that would be ensuring that this feature is implementable on top of std.process, which I think is the case thanks to Pid exposing osHandle.

std/process.d Outdated Show resolved Hide resolved
@schveiguy
Copy link
Member

There's not a portable option for Linux AFAIK. You need to use signals with another thread which can send the signal. I don't know if this is something we would want to implement in Phobos. I doubt there are going to be any advancements in Linux/Unix that allow for a wait with a timeout.

So if we add this, it will likely be permanently Windows-only. And I think that's fine, we should expose the OS functionality available for the OS you are using. For example, we have config options which are OS specific (though those are just ignored on other OSes).

@schveiguy
Copy link
Member

schveiguy commented Jun 12, 2020

Hm... I take it back (a bit), SIGALRM can be used without threads to implement this. So perhaps it is possible. But the granularity is possibly in seconds, which is not ideal.

@CyberShadow
Copy link
Member

Hm... I take it back (a bit), SIGALRM can be used without threads to implement this. So perhaps it is possible.

Internet says SIGALRM is a bad idea due to race conditions.

BTW, this task is achievable in a cross-platform manner without any changes in Phobos by creating a pipe and passing one end to the process, and then using select or equivalent with a timeout on the other end to wait until the pipe is closed. (The pipe handle will be propagated to further child processes, which may or may not be what is needed.)

@schveiguy
Copy link
Member

schveiguy commented Jun 12, 2020

It highly depends on the child process. The process could close the pipe without exiting because it doesn't plan to do any output.

So I don't think it's portable for all situations.

Edit: oh you mean a special pipe for this purpose. Hm... it's possible. You could fork/exec and set the noinherit on the pipe before the exec.

Edit 2: Nevermind, I was thinking of FD_CLOEXEC, which doesn't help.

@schveiguy
Copy link
Member

Internet says SIGALRM is a bad idea due to race conditions.

Yeah, the more I look at it, the less nice it looks.

@CyberShadow
Copy link
Member

The process could close the pipe without exiting because it doesn't plan to do any output.

It doesn't have to be STDOUT_FILENO, can be an arbitrary handle. But, your point does still stand, the process technically could iterate over open fds and close them - I imagine some programs do that as part of daemonification.

@MoonlightSentinel
Copy link
Contributor Author

Could also consider sigtimedwait, e.g. as proposed here

@ghost
Copy link

ghost commented Jun 12, 2020

How about a control loop with Thread.sleep() ? This is actually what I occasionaly use when I have to do a non blocking wait on several Pid. So something like this:

auto waitTimeOut(Pid p, Duration timeout)
{
    Duration ellapsted;
    ReturnType!trywait result;
    while (ellapsted < timeout)
    {
        result =  tryWait(p);
        if (result.terminated)
            return r;
        Thread.sleep(10.msecs);
        ellapsted += 10.msecs;
    }
    return result;
}

that would work for linux as well

@MoonlightSentinel
Copy link
Contributor Author

Busy waiting is inefficient because the parent keeps consuming resources and also imprecise because you wait way too long depending on your sleep duration.

I could kinda see it as an "if all else fails" solution but i don't think it's appropriate for phobos.

@schveiguy
Copy link
Member

sigtimedwait probably isn't usable, that example doesn't seem to account for the possibility of waiting for a specific child, or in a thread.

@CyberShadow
Copy link
Member

I think the idea is that after every SIGCHLD, you check (as in tryWait) to see if it is the waited-on process which exited, or something else.

@schveiguy
Copy link
Member

I would expect the SIGCHLD to immediately re-fire if you don't wait for the child that caused the signal. Is that not true?

@CyberShadow
Copy link
Member

Sorry, I don't know. You may be right.

@MoonlightSentinel
Copy link
Contributor Author

Anybody got an idea how to fix LDC-Win32 on Azure?

.\unittest.exe
ERROR: Input redirection is not supported, exiting the process immediately.

@MoonlightSentinel MoonlightSentinel force-pushed the timed-wait branch 2 times, most recently from 4e3ae4f to d3f0c6c Compare March 8, 2021 21:16
@RazvanN7
Copy link
Collaborator

@MoonlightSentinel Maybe it was a spurious failure? Maybe you can rebase this and see what the autotester has to say?

@MoonlightSentinel
Copy link
Contributor Author

MoonlightSentinel commented Apr 21, 2021

Don't think so, the failure persistet across several runs. I'll try a modified test case.

`wait(Pid, Duration)` allows to wait until the process terminates
or the timeout expires.

It's currently windows-only because a decent posix implementation
is quite difficult and this allows us to sort out the API beforehand.
Copy link
Collaborator

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@RazvanN7 RazvanN7 added Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Apr 22, 2021
@dlang-bot dlang-bot merged commit 9d5db86 into dlang:master Apr 26, 2021
@MoonlightSentinel MoonlightSentinel deleted the timed-wait branch April 26, 2021 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants