Wait-Event - Adding -TimeSpan #19705

StartAutomating · 2023-05-24T02:44:35Z

Adding a -TimeSpan parameter to Wait-Event (Fixes #19704). This allows more granularity in waiting on events.

Additionally, changed the polling interval within the command to be 1/100th of the timespan.

These changes improve the performance of Wait-Event substantially, as Wait-Event was always going to wait at least 1/5th of a second.

Due to requests for this to include an argument transformation attribute, this also fixes #20112 .

PR Checklist

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WaitEventCommand.cs

SteveL-MSFT

tests are missing

…PowerShell#19704)

StartAutomating · 2023-05-24T05:09:04Z

tests are missing

@SteveL-MSFT added a test

rhubarb-geek-nz · 2023-05-24T06:05:41Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WaitEventCommand.cs

                        break;
                }

-                received = _eventArrived.WaitOne(200);
+                received = _eventArrived.WaitOne((int)(_timeoutTimespan.TotalMilliseconds / 100));


The wait time should be the number of milliseconds until the expiry time occurs. So you wait exactly once, not a hundred times. So before entering the loop determined the point in time when the expiry will occur, each time round the loop calculate the number of milliseconds between now and the expiry time. If the number of milliseconds is negative then you have expired.

If there is no expiry time set then WaitOne() should be used with no expiry time

Unfortunately, it still has to check after each wait to see if the event has arrived. Because this is called in a relatively tight loop, this will probably happen more than once.

After an even comes it, the cmdlet checks to see if it is the correct -SourceIdentifier. If it was, then Wait-Event returns, otherwise, it checks again.

That should not be right. It should be completely time independent except for the timeout. The AutoResetEvent _eventArrived should be triggered in NotifyEvent which is called by ReceivedEvents_PSEventReceived which is subscribed by Events.ReceivedEvents.PSEventReceived += ReceivedEvents_PSEventReceived;

So if there is no timeout then you should be able to use WaitOne() because the event will be triggered when there is a change.

It should not be polling every some arbitrary time period, it should timeout when there is nothing to do, and it should wake up in the loop when an event is received to match the new event.

@rhubarb-geek-nz noted, with two additional points:

The case I'm most concerned about is the rapid timeout or return of a single upcoming event (aka, providing a -Timespan). I believe doing a slightly more efficient job of waiting forever does not help that scenario.

When the event is triggered, .WaitOne should return as soon as possible. The timespan on .WaitOne is how long to wait before timing out.

If the PowerShell Team requests this change, we can address this. Otherwise, I'd prefer to keep the code change small and the level of intrigue to a minimum, so as to maximize chances of a speedy integration.

iSazonov · 2023-05-24T06:44:26Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WaitEventCommand.cs

+        /// Negative values mean never timeout.
+        /// </summary>
+        [Parameter]        
+        public TimeSpan TimeSpan


I guess we could change type of Timeout parameter to TimeSpan and add type converter (seconds to TimeSpan) to mitigate the breaking change

It'd be nice if we could, and maybe I'm missing some type converter magic we could use.

When casting [Timespan] from an [int], it casts to ticks, not seconds.

Under the covers I'm turning -TimeOut into -Timespan.

One other thought to avoid having a new parameter would be having -TimeOutSec become a [double], which would allow us to pass, say 0.5.

Unfortunately, I believe this is likely to create some accidental back-compat errors, as scripts with a timeout of 0.25 would run fine on later versions and not wait at all on earlier ones (because of rounding).

IMHO, having a new parameter is a much better hint that it's a newer version.

IMHO, having a new parameter is a much better hint that it's a newer version.

If we can use an existing parameter, we should do so. Otherwise it will mislead users, it's bad UX.

PowerShell can do clever things with parameters. You can find examples by searching for ArgumentTransformationAttribute.

@SteveL-MSFT what's your opinion: would you rather a new parameter or transform the old one?

Let me bring this up to the Cmdlets WG

@SteveL-MSFT / the working group:

I'd like to see this fix go in, as it would fairly immediately improve the performance of working modules.

I'm fairly confident that having two (or more) parameters would be the right way to go here. To restate the reasons:

Making -TimeoutSec a [double] would break back-compat in a weird way, due to rounding.

Making -TimeOutSet a [TimeSpan] would create weird casting errors, as ints become 'ticks' in [TimeSpan]s

Making something more complicated to make [TimeSpan] magically castable from [int] might have been nice if it was PowerShell 1, but, as it stands, adding something like a type converter here would potentially break a lot of people's code (and complicate this pull request).

Adding a -CancellationToken would be desirable, and I believe it should be another PR.

I am trying to keep this PR concise, as I expect to do future work in this space, and I am trying to do one PR per issue.

If there is a solid blocking reason to pend on this, please let me know what it might be, and I'll be happy to make any modifications you need.

To restate the rationale, in the hopes of a speedy resolution:

No one should have to wait a second to find out things didn't work. The faster one can time out a wait for an event that should be effectively immediate, the faster the rest of the code will work.

Please let me know if any adjustments are needed.

Hope this helps,

James

iSazonov · 2023-05-24T06:50:54Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WaitEventCommand.cs

                        break;
                }

-                received = _eventArrived.WaitOne(200);
+                received = _eventArrived.WaitOne((int)(Math.Abs(_timeoutTimespan.TotalMilliseconds) / 100));


It is not correct. Timeout can accept -1 and no need to divide milliseconds.

ghost · 2023-06-02T02:00:50Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

SteveL-MSFT · 2023-06-21T16:39:20Z

@PowerShell/wg-powershell-cmdlets reviewed this, we agree that the desired behavior is for -Timeout to take a [TimeSpan] and having a parameter type convertor that accepts an int (to maintain existing behavior) or a double, so that partial seconds could be specified rather than having an additional parameter.

ghost · 2023-06-29T02:00:55Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

microsoft-github-policy-service · 2023-08-01T08:59:09Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

SteveL-MSFT · 2023-08-13T03:24:47Z

@StartAutomating can you review the CodeFactor issues? Also, CI isn't passing.

pull-request-quantifier-deprecated · 2023-08-13T21:48:04Z

This PR has 86 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Small
Size       : +68 -18
Percentile : 34.4%

Total files changed: 3

Change summary by file extension:
.cs : +50 -9
.ps1 : +18 -9

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

StartAutomating · 2023-08-13T22:59:12Z

@SteveL-MSFT

@StartAutomating can you review the CodeFactor issues? Also, CI isn't passing.

Fixed.

Please let me know if you'd like some tests on the ArgumentTransform attribute as well.

JamesWTruher

looks pretty good - just looking for 1 additional test for the transform

JamesWTruher · 2023-08-14T19:29:33Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Wait-Event.Tests.ps1

+			$stopwatch.ElapsedMilliseconds | Should -BeGreaterThan 200
+			$stopwatch.ElapsedMilliseconds | Should -BeLessThan 300
+		}
+	}


we should probably have an additional test for the transform. Replicating the test at line 16 with a string value of "00:00:00.25" should do it. I don't think we need a test for a non-convertable value unless you want to validate that you're getting a parameter bind/transform fail error

@JamesWTruher a string value would be good.

It's a scenario where we'd need to use a -TransformScript, not just a property name (we'd check to see if it could be a timespan, then pull out the total seconds).

What is the preferred mechanism for using a ScriptBlock as an attribute value within the PowerShell codebase?

Also, Joel Brings up a fair point:

#20112 (comment)

Additionally, I feel it would be very nice if we could make $This in the Transform be the command the ScriptBlock is tied to (but, alas, don't quite know how we'd be able to know that within an attribute). It may also be somewhat nice to have a string property used for metadata about the transform (e.g. a description).

Any ideas on how to make these happen?

@SeeminglyScience had suggested these be two separate PRs (one for the transform attribute, one for Wait-Event's changes).

As eager as I am to see Wait-Event work with subsecond timeouts (it limits functionality of obs-powershell+ScriptDeck), please let me know if you'd prefer these be separate PRs.

@StartAutomating I would agree with @SeeminglyScience that these should be two separate PRs starting with the ArgumentTransform (thanks for taking the feedback and creating it!). It should probably have its own set of tests independent of this cmdlet. Once that one is merged, this PR should be simplified to just use that attribute.

microsoft-github-policy-service · 2023-08-28T22:41:25Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

microsoft-github-policy-service · 2023-08-28T22:41:25Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

StartAutomating · 2023-09-12T08:48:25Z

@SteveL-MSFT thanks for the approval, however, I was still planning on making the [ArgumentTransform] a bit better.

If this isn't fully merged, please let me do that this week.

SteveL-MSFT · 2024-02-12T17:04:51Z

@StartAutomating removing the community day label until you are ready

pull-request-quantifier-deprecated · 2024-02-21T22:46:36Z

This PR has 137 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Medium
Size       : +119 -18
Percentile : 47.4%

Total files changed: 5

Change summary by file extension:
.cs : +60 -9
.ps1 : +59 -9

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

pull-request-quantifier-deprecated · 2024-03-01T20:30:14Z

This PR has 144 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Medium
Size       : +124 -20
Percentile : 48.8%

Total files changed: 6

Change summary by file extension:
.cs : +60 -9
.ps1 : +64 -11

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

Wait-Event - Adding -TimeSpan (Fixes PowerShell#19704)

0c92d0e

StartAutomating requested a review from PaulHigin as a code owner May 24, 2023 02:44

pull-request-quantifier-deprecated bot added the Extra Small label May 24, 2023

ghost assigned TravisEz13 May 24, 2023

Wait-Event - Fixing spacing as requested by bot (re PowerShell#19704)

0d184bd

iSazonov reviewed May 24, 2023

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WaitEventCommand.cs Show resolved Hide resolved

iSazonov reviewed May 24, 2023

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WaitEventCommand.cs Outdated Show resolved Hide resolved

SteveL-MSFT requested changes May 24, 2023

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 24, 2023

SteveL-MSFT added the CommunityDay-Small A small PR that the PS team has identified to prioritize to review label May 24, 2023

Wait-Event - Using absolute value of timespan in case of negatives (re …

1952255

…PowerShell#19704)

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 24, 2023

Adding tests for Wait-Event -TimeSpan (re PowerShell#19704)

863483c

rhubarb-geek-nz reviewed May 24, 2023

View reviewed changes

iSazonov reviewed May 24, 2023

View reviewed changes

ghost added the Review - Needed The PR is being reviewed label Jun 2, 2023

StevenBucher98 added the PowerShell-Docs needed The PR was reviewed and a PowerShell Docs update is needed label Jun 5, 2023

SteveL-MSFT added the WG-Cmdlets general cmdlet issues label Jun 19, 2023

ghost removed the Review - Needed The PR is being reviewed label Jun 21, 2023

ghost added the Review - Needed The PR is being reviewed label Jun 29, 2023

SteveL-MSFT added WG-Reviewed A Working Group has reviewed this and made a recommendation Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Jul 5, 2023

daxian-dbw removed the Review - Needed The PR is being reviewed label Jul 17, 2023

microsoft-github-policy-service bot added the Stale label Aug 1, 2023

microsoft-github-policy-service bot removed the Stale label Aug 12, 2023

Aliasing -TimeoutSec to -TimeSpan Wait-Event (re PowerShell#19704)

8c258ba

microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 13, 2023

James Brundage added 3 commits August 13, 2023 13:49

Wait-Event - Aligning coding style to CodeFactor

df3f162

Wait-Event - Fixing tests

b21b546

Wait-Event - Aligning coding style to CodeFactor

7e4c3ae

JamesWTruher suggested changes Aug 14, 2023

View reviewed changes

microsoft-github-policy-service bot added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Aug 14, 2023

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Aug 28, 2023

SteveL-MSFT approved these changes Sep 11, 2023

View reviewed changes

SteveL-MSFT removed the CommunityDay-Small A small PR that the PS team has identified to prioritize to review label Feb 12, 2024

microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Feb 12, 2024

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Feb 19, 2024

James Brundage added 2 commits February 21, 2024 14:22

Merge remote-tracking branch 'origin/master' into Wait-Event-Improvement

9b8be4c

Using PSTransform Attribute in Wait-Event and adding test

e46488d

StartAutomating requested a review from daxian-dbw as a code owner February 21, 2024 22:46

pull-request-quantifier-deprecated bot added Medium and removed Small labels Feb 21, 2024

StartAutomating mentioned this pull request Feb 21, 2024

Wait-Event should allow more granularity #19704

Open

fix: Fixing Type Accelerator Test ( re PowerShell#20112 )

fcd9b99

Wait-Event - Adding -TimeSpan #19705

Are you sure you want to change the base?

Wait-Event - Adding -TimeSpan #19705

Conversation

StartAutomating commented May 24, 2023 • edited

PR Checklist

SteveL-MSFT left a comment

Choose a reason for hiding this comment

StartAutomating commented May 24, 2023

rhubarb-geek-nz May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 2, 2023

SteveL-MSFT commented Jun 21, 2023 • edited

ghost commented Jun 29, 2023

microsoft-github-policy-service bot commented Aug 1, 2023

SteveL-MSFT commented Aug 13, 2023

pull-request-quantifier-deprecated bot commented Aug 13, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

StartAutomating commented Aug 13, 2023

JamesWTruher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

microsoft-github-policy-service bot commented Aug 28, 2023

microsoft-github-policy-service bot commented Aug 28, 2023

StartAutomating commented Sep 12, 2023

SteveL-MSFT commented Feb 12, 2024

pull-request-quantifier-deprecated bot commented Feb 21, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

pull-request-quantifier-deprecated bot commented Mar 1, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

StartAutomating commented May 24, 2023 •

edited

rhubarb-geek-nz May 24, 2023 •

edited

SteveL-MSFT commented Jun 21, 2023 •

edited