Start-Process: add parameter -ExitTimeout #5471

stuajnht · 2017-11-16T00:15:23Z

This is to resolve #5185 by adding a Timeout parameter to Start-Process.

If the timeout is reached and the process hasn't finished, the process is stopped.

To perform the stopping of processes, the Stop-Process cmdlet is invoked. This is done to prevent repeating any checks needed when killing a process.

Before the process is stopped, a "search" is attempted to find all descendant processes to hopefully avoid any orphaned processes remaining active. This uses WMI on Windows and the ps command on UNIX-like systems, as this provided the most reliable way for cross-platform support.

As mentioned in the contributing guidelines, this change may warrant a note in the changelog (I am unsure where to put it).

[feature] If the timeout is reached and the process hasn't finished, the process is stopped.

msftclas · 2017-11-16T00:15:31Z

All CLA requirements met.

SteveL-MSFT

Thanks for the contribution!

SteveL-MSFT · 2017-11-16T01:53:46Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+        /// </summary>
+        [Parameter]
+        [Alias("TimeoutSec")]
+        [ValidateNotNullOrEmpty]


Is there a specific reason for this upper limit?

Usually we use [ValidateRange(0, Int32.MaxValue)] but now we have [ValidateRange(ValidateRangeKind.Positive)]

@SteveL-MSFT No reason, just that the Wait-Process cmdlet has this limit so I used the same range.
@iSazonov I have changed it to [ValidateRange(ValidateRangeKind.Positive)].

SteveL-MSFT · 2017-11-16T02:00:12Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+            string message = StringUtil.Format(ProcessResources.StartProcessTimeoutExceeded, process.ProcessName);
+            ErrorRecord er = new ErrorRecord(new TimeoutException(message), "StartProcessTimeoutExceeded", ErrorCategory.OperationTimeout, process);
+
+            StopProcessCommand stop = new StopProcessCommand();


Seems like Stop-Process should have a switch called -IncludeChildProcesses and you would use that here

Ideally. As there currently isn't one, I implemented the GetProcessTreeIds function to do this role.

If this code is accepted, I had planned to open an issue / PR about somehow integrating this function into the Stop-Process cmdlet in the future (so as not to change too much code in this PR), instead of it being located in Start-Process.

SteveL-MSFT · 2017-11-16T02:19:25Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+        [Parameter]
+        [Alias("TimeoutSec")]
+        [ValidateNotNullOrEmpty]
+        [ValidateRange(1, 32767)]


I think that Timeout is a bit misleading as it's not really a timeout on starting the process, but on how long it's expected to run. Perhaps calling this -WaitForExitTimeout would be more accurate? cc @HemantMahawar @joeyaiello

-ExitTimeout
Also we could type of -Wait from switch to int to accept timeout.

Agreed; -ExitTimeout makes more sense. Would changing -Wait to an int cause any problems with existing scripts?

We should probably leave -Wait as-is for compat reasons. -ExitTimeout works for me.

SteveL-MSFT · 2017-11-16T02:20:22Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+                            jobObject.WaitOne(_waithandle, _timeout);
+                            if (!process.HasExited)
+                            {
+                                StopProcessOnTimeout(process);


I'm wondering if we actually want to stop the process on timeout by default. I think it might be sufficient to return a non-terminating error so that -PassThru returns the process so the user can terminate it if so desired? cc @HemantMahawar @joeyaiello

From a user's perspective, I'd expect PowerShell to stop the process for me because this is how at least I would use this parameter. Even if it is just a naming problem, a -Timeout like implementation would be more useful to me than a -WaitForExitTimeout implementation.

I wonder why we're actually making this change if the following works fine Start-Process -PassThru | Wait-Process -Timeout? It seems we duplicate a code.
I'd expect that we enhance Wait-Process cmdlet with -PassThru to get Start-Process -PassThru | Wait-Process -Timeout 10 -PassThru | Stop-Process.

@SteveL-MSFT @bergmeister I thought about both of these ways when implementing this, but settled on stopping the process on timeout, similar to https://ss64.com/bash/timeout.html. The reasoning is if I started a process and gave it a time limit, I would expect it to stop if the limit is reached, rather than just let me know it had gone over the limit. Is it worth adding a switch -StopOnExitTimeout (or similar)? Or, should the below suggestion be used?

@iSazonov I see your point, and didn't think of it being run this way. I suppose adding the -ExitTimeout parameter is about simplifying the amount of cmdlets needed. Would you prefer me to implement -PassThru for Wait-Process instead?

SteveL-MSFT · 2017-11-16T02:21:07Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+
+                processRelationships = ps.StandardOutput.ReadToEnd();
+
+                ps.WaitForExit();


Is there a potential this could not exit?

Agreed. Shall add a few seconds wait before returning, just in case.

SteveL-MSFT · 2017-11-16T02:21:39Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+                processRelationships = ps.StandardOutput.ReadToEnd();
+
+                ps.WaitForExit();
+                ps.Close();


Should we check the exit code?

I had assumed this would either succeed or throw an error. Shall add a check just in case.

SteveL-MSFT · 2017-11-16T02:24:40Z

test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1

+        }
+
+        # Find where test/powershell is so we can find the testexe command relative to it
+        $powershellTestDir = $PSScriptRoot


You shouldn't have to search for testexe, I think the current pattern is to expect using Start-PSPester which builds testexe and puts it into the path

Sorry, didn't realise it worked that way. Shall change to just use textexe.

iSazonov

I wonder why we're actually making this change if the following works fine Start-Process -PassThru | Wait-Process -Timeout? It seems we duplicate a code.
I'd expect that we enhance Wait-Process cmdlet with -PassThru to get Start-Process -PassThru | Wait-Process -Timeout 10 -PassThru | Stop-Process.

iSazonov · 2017-11-16T14:47:33Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+        /// </summary>
+        [Parameter]
+        [Alias("TimeoutSec")]
+        [ValidateNotNullOrEmpty]


Usually we use [ValidateRange(0, Int32.MaxValue)] but now we have [ValidateRange(ValidateRangeKind.Positive)]

iSazonov · 2017-11-16T14:48:31Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+        [Parameter]
+        [Alias("TimeoutSec")]
+        [ValidateNotNullOrEmpty]
+        [ValidateRange(1, 32767)]


-ExitTimeout
Also we could type of -Wait from switch to int to accept timeout.

iSazonov · 2017-11-16T14:56:02Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+                _timeout = value * 1000;
+                _timeoutSpecified = true;
+            }
+        }


We can use autogenerated property:

public int Timeout { Get; Set; } = Threading.Timeout.Infinite;

Thanks. I was not aware of doing it this way.

iSazonov · 2017-11-16T15:06:35Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

        [Parameter]
        public SwitchParameter Wait { get; set; }

+        /// <summary>


Please follow common convention and add final dot in public comments.

Added. The other public comments in Start-Process didn't have a dot, so I followed the same format as those.

iSazonov · 2017-11-16T15:11:41Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

            }

-            if (Wait.IsPresent)
+            if (Wait.IsPresent || _timeoutSpecified)


If we'll use autogenerated property we can check Timeout != Threading.Timeout.Infinite

Have changed as requested

iSazonov · 2017-11-16T15:25:31Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

                {
                    if (!process.HasExited)
                    {
 #if UNIX


We should remove two code paths and use public bool WaitForExit(int milliseconds); for all plaforms.

I'm sorry, I don't quite understand what you are asking here. Are you saying to remove the ProcessCollection / jobObject and just have a WaitForExit for all platforms? e.g.

if (!process.WaitForExit(ExitTimeout)) { StopProcessOnTimeout(process); }

From what I can see, the Windows specific code keeps track of any child processes that are created, which doesn't happen for UNIX. I am unsure of the reasoning behind this, as the earliest commit on GitHub for these lines does not contain any helpful information.

iSazonov · 2017-11-16T15:28:33Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+                            jobObject.WaitOne(_waithandle, _timeout);
+                            if (!process.HasExited)
+                            {
+                                StopProcessOnTimeout(process);


I wonder why we're actually making this change if the following works fine Start-Process -PassThru | Wait-Process -Timeout? It seems we duplicate a code.
I'd expect that we enhance Wait-Process cmdlet with -PassThru to get Start-Process -PassThru | Wait-Process -Timeout 10 -PassThru | Stop-Process.

[feature] Based on PR feedback, also: * Skipped searching for `testexe`, as `Start-PSPester` puts it into the path * Set the upper limit for `ExitTimeout` to be a positive range, rather than 32767 * Used autogenerated property for `ExitTimeout` * Set a timeout and checked the exit code for the `ps axo` process * Used common coding convention for comments

stuajnht · 2017-11-17T23:34:35Z

Hi all,

Thanks for your review feedback. I have made a number of changes based on it:

Changed Timeout parameter to ExitTimeout
Skipped searching for testexe, as Start-PSPester puts it into the path
Set the upper limit for ExitTimeout to be a positive range, rather than 32767
Used autogenerated property for ExitTimeout
Set a timeout and checked the exit code for the ps axo process
Used common coding convention for comments

Any questions that I have are in the relevant feedback comments.

SteveL-MSFT · 2017-11-17T23:57:19Z

The concept is definitely a good one and something needed. I'd like @PowerShell/powershell-committee to review the design to make sure it fits with PowerShell norms.

iSazonov · 2017-11-18T09:17:32Z

As I said above I'd prefer to see Start-Process -PassThru | Wait-Process -Timeout 10 -PassThru | Stop-Process.
Another thought is that -Wait and -ExitTimeout is duplicates. We'd accept a slight breaking change -Wait | -Wait <timeout> where -Wait is infinite timeout. It is breaking only -Wait:$true that is rare case.
Another thought is - can there be other timeouts? If no we'd use -Timeout name.

mattpwhite · 2017-11-22T19:25:12Z

Is this a race? It looks like PIDs are being collected as integers (as opposed to handles that might prevent re-use), then later those PIDs are killed, but there's nothing to say that the PID hasn't been reused by the OS for a different process at that point, or is there?

mklement0 · 2017-11-23T16:24:30Z

@stuajnht: Consistency of parameter names across cmdlets is important, so I agree with @iSazonov that we should revert to -Timeout.

@iSazonov: Being able to specify a timeout value directly is a welcome convenience that we should add to other cmdlets as well - see #5434 and #5433

PowerShell lacks the concept of an optional argument value altogether, and I suggest not opening that can of worms with something -Wait | -Wait <timeout>. (If anything, to avoid ambiguity, it would have to have the form -Wait | -Wait:<timeout>, analogous to how POSIX shells do it.)

To me, there's nothing wrong with a separate -Timeout parameter.

stuajnht · 2017-11-26T20:18:11Z

@mklement0 @iSazonov I am happy to change the parameter back to -Timeout if no-one else has any objections about the parameter name needing to be clarified about what it does.

@mattpwhite After trying with a variety of programs, I believe that you are right. When I was coding this, killing the parent process would keep the child processes running. As was mentioned in one of the comments, ideally the Stop-Process cmdlet would have the ability to terminate child processes. My idea was to include something in this PR to prevent child processes being leftover when the timeout occurs, but I realise now it should be in its own PR instead.

@SteveL-MSFT Has anything been considered if the process should be stopped on the timeout being reached, or if it should just warn the user?

Thanks all, and I'll update the PR once I have more information.

mklement0 · 2017-11-26T20:59:14Z

@stuajnht:

if the process should be stopped on the timeout being reached

To me, that calls for another switch: -StopOnTimeout; by default, no action (other than the timeout error) should be taken.

SteveL-MSFT · 2017-11-27T17:55:32Z

@stuajnht I'll try to get the committee to review this today

SteveL-MSFT · 2017-11-29T23:28:07Z

@PowerShell/powershell-committee reviewed this and would like the parameter to be called -MaximumDuration and validate that when used with -PassThru, the dead process object should be emitted. -MaximumDuration implies -Wait and no requirement to make them mutually exclusive. We agree it's a good idea to add -PassThru to Wait-Process to support advanced usage scenarios as a separate issue/PR.

mklement0 · 2017-11-29T23:37:37Z

May I ask what the rationale is for not sticking with the well-established -Timeout name for the parameter (used in several other cmdlets)?

Aside from this inconsistency, "duration" smacks of [timespan].

mklement0 · 2017-11-29T23:55:13Z

Implying -Wait when a timeout is specified definitely makes sense, but, again ... uh ... calling a timeout a timeout makes sense too.

SteveL-MSFT · 2017-11-30T00:52:46Z

@mklement0 the main push back on calling it Timeout is that it's not a timeout on the starting of a process, but the max lifetime allowed for the started process. Duration sounded better than Lifetime.

mklement0 · 2017-11-30T02:33:34Z

@SteveL-MSFT:

I appreciate your letting me know.

I don't think there's any point in distinguishing between launch duration and execution duration - they should be treated as a single timeout, starting at the launch point in time.

If there's a technical reason for not being able to enforce the timeout during the launch phase a simple note in the documentation will do.

Even if you did want to make the distinction you're suggesting, -MaximumDuration is no less ambiguous than -Timeout: duration of what?

iSazonov · 2017-11-30T05:00:35Z

I'd prefer to see unified parameter names. These parameters (Timeout) are easier to discover and remember than the unique parameters (MaximumDuration) in each cmdlet.

SteveL-MSFT · 2017-11-30T23:52:08Z

@mklement0 @iSazonov appreciate all the feedback and you both make valid points, will bring it back up

SteveL-MSFT · 2017-12-06T23:09:16Z

@PowerShell/powershell-committee revisited this and would be ok with -ExecutionTimeout. In general, documentation should not be required to understand the intent or purpose of a parameter. The name should be self documenting as much as possible without being overly verbose. Just -Timeout is ambiguous of the purpose, but -ExecutionTimeout is concise and reasonable.

TravisEz13 · 2018-02-08T21:54:52Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+        [Parameter]
+        [ValidateNotNullOrEmpty]
+        [ValidateRange(ValidateRangeKind.Positive)]
+        public int ExitTimeout { get; set; } = Timeout.Infinite;


needs to be updated per committee review

stale · 2018-03-16T23:47:21Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. Thank you for your contributions.

iSazonov · 2018-03-19T06:34:55Z

@stuajnht Could you please continue?

TravisEz13 · 2018-03-19T20:46:54Z

@iSazonov Please consider the bots comment the request to continue. By commenting, you are defeating the bot.

iSazonov · 2018-03-20T04:32:09Z

@TravisEz13 Thanks for clarify. I thought the bot only counted commits.
I think the PR is useful and we could grab and continue.

TravisEz13 · 2018-03-20T17:29:07Z

@iSazonov But a closed PR can be continued as well as long as the owner doesn't delete the branch. The bot will label all of them as review-abandoned so we can query for them. If you think the wording of the message of the bot needs to be updated, create a PR or just file an issue with a proposal and mention me.

iSazonov · 2018-03-21T06:16:17Z

@TravisEz13 I open Issue #6446 with suggestion.

stale · 2018-04-20T06:22:17Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

stale · 2018-04-30T07:21:07Z

This PR has be automatically close because it is stale. If you wish to continue working on the PR, please first update the PR, then reopen it.
Thanks again for your contribution.
Community members are welcome to grab these works.

Start-Process: add parameter 'Timeout'

bb5db45

[feature] If the timeout is reached and the process hasn't finished, the process is stopped.

stuajnht requested review from adityapatwardhan and daxian-dbw as code owners November 16, 2017 00:15

daxian-dbw requested review from JamesWTruher, SteveL-MSFT and anmenaga November 16, 2017 01:33

SteveL-MSFT requested changes Nov 16, 2017

View reviewed changes

iSazonov reviewed Nov 16, 2017

View reviewed changes

stuajnht changed the title ~~Start-Process: add parameter 'Timeout'~~ Start-Process: add parameter 'ExitTimeout' Nov 17, 2017

SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Nov 17, 2017

iSazonov mentioned this pull request Nov 22, 2017

Fix Set-Service -Status Stopped runs failed on the service with dependencies #5525

Merged

9 tasks

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Nov 29, 2017

SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Nov 30, 2017

SteveL-MSFT removed the Committee-Reviewed PS-Committee has reviewed this and made a decision label Nov 30, 2017

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Dec 6, 2017

daxian-dbw assigned TravisEz13 Feb 1, 2018

TravisEz13 changed the title ~~Start-Process: add parameter 'ExitTimeout'~~ Start-Process: add parameter -ExitTimeout Feb 8, 2018

TravisEz13 requested changes Feb 8, 2018

View reviewed changes

stale bot added the Stale label Mar 16, 2018

stale bot removed the Stale label Mar 19, 2018

stale bot added the Stale label Apr 20, 2018

stale bot closed this Apr 30, 2018

davidBar-On mentioned this pull request May 23, 2021

Receive-Job should have a -Timeout parameter to complement -Wait #5433

Closed


		processRelationships = ps.StandardOutput.ReadToEnd();

		ps.WaitForExit();

Start-Process: add parameter -ExitTimeout #5471

Start-Process: add parameter -ExitTimeout #5471

Uh oh!

Conversation

stuajnht commented Nov 16, 2017

Uh oh!

msftclas commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuajnht commented Nov 17, 2017

Uh oh!

SteveL-MSFT commented Nov 17, 2017

msftclas commented Nov 16, 2017 •

edited

Loading

bergmeister Nov 16, 2017 •

edited

Loading

mklement0 commented Nov 23, 2017 •

edited

Loading

SteveL-MSFT commented Nov 29, 2017 •

edited

Loading