Don't write DNS resolution errors on Test-Connection -Quiet #12204

vexx32 · 2020-03-25T20:20:42Z

PR Summary

We don't need to write a DNS resolution error if the user uses -Quiet; it's the same as a False result in that case and can be treated the same.

Also, currently we're not able to respond to StopProcessing() between pings due to the Dns.GetHostEntry() method blocking. Fix is to implement a GetHostEntryWithCancel() method that factors in a cancellation token.

PR Context

Resolves #12147
Resolves #11160

PR Checklist

+ Add GetHostEntryWithCancel method and _cancel member. + Add cancellation for DNS lookups.

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

iSazonov · 2020-03-26T08:26:30Z

test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1

+        It 'does not emit errors for an unresolvable address when using -Quiet' {
+            Test-Connection -Quiet -TargetName "fakeHost" -Count 1 | Should -BeFalse
+        }


The test name is seem not right.

I'll give it another pass, I missed a couple things here, it seems.

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

test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1

Co-Authored-By: Ilya <darpa@yandex.ru>

test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1

…onnection.Tests.ps1 Co-Authored-By: Ilya <darpa@yandex.ru>

daxian-dbw · 2020-03-26T20:37:03Z

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

        protected override void StopProcessing()
        {
            _sender?.SendAsyncCancel();
+            _dnsLookupCancel?.Cancel();


_dnsLookupCancel is never null

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

daxian-dbw · 2020-03-26T20:44:38Z

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

                }
+                catch (PipelineStoppedException p)
+                {
+                    throw p;


You should use throw; here.

Maybe a conditional catch is better?

catch (Exception ex) when ((ex as PipelineStoppedException) == null) { .... }

Didn't know that was a thing. Is there a significant difference?

Which one you are asking about?

for the throw, it will keep the original stack trace; while throw p will rewrite the stack trace.
for the conditional catch block, it's slightly efficient because it avoids looking for a handler for the PipelineStoppedException. Although the efficiency doesn't matter here, it looks more concise :)

Poked at this one for a bit and changed my mind a couple times, but it looks like the conditional catch block is about the same as just doing a general catch with an if statement when it comes down to it. Separate catch block seems a little tidier.

I'm fine keeping the catch (PipelineStoppedException p) block, as long as you change throw p to throw;.
The main benefit of conditional exception is to avoid unnecessary search of exception handlers, and of course, avoid running a handler.

daxian-dbw · 2020-03-26T20:46:58Z

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

            return true;
        }

+        private IPHostEntry GetHostEntryWithCancel(string targetNameOrAddress)


Maybe name the method similarly to SendCancellablePing? GetCancellableHostEntry? 😄
Also, a minor thing, I recommend to pass in the cancellation token as parameter.

No worries, I'll sort it out. 🙂

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

vexx32 · 2020-03-27T01:39:38Z

I think that should address all your comments @daxian-dbw. 🙂

Let me know if I misunderstood anything; I'm still pretty new to fiddling with cancellation tokens and whatnot. ^^

vexx32 · 2020-03-27T01:44:33Z

🤔 realizing that returning false from this method is still gonna be a bit janky. Need to handle it and actually write false values back in the other methods too... Hm. That's okay, shouldn't be too tricky to manage.

EDIT: I think that ought to sort it out. Let me know if you have any concerns with how I'm handling it. 😊

+ Since we're not emitting errors for these cases, we need to make sure that we give the users an appropriate -Quiet response. + In most cases, this is just false. -MtuSize is a special case that typically outputs integers, so write -1 to indicate failure to connect there.

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

test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1

…onnection.Tests.ps1 Co-Authored-By: Ilya <darpa@yandex.ru>

iSazonov · 2020-03-27T16:33:11Z

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


        private static byte[]? s_DefaultSendBuffer;

+        private CancellationTokenSource _dnsLookupCancel = new CancellationTokenSource();


From Codacy:

Suggested change

private CancellationTokenSource _dnsLookupCancel = new CancellationTokenSource();

private readonly CancellationTokenSource _dnsLookupCancel = new CancellationTokenSource();

Huh, I thought I... ah, I must have taken it out again at some point. Thanks!

…nt/TestConnectionCommand.cs Co-Authored-By: Ilya <darpa@yandex.ru>

ghost · 2020-04-23T18:02:14Z

🎉v7.1.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

vexx32 added 2 commits March 25, 2020 15:31

🐛 Don't write hostname resolve error on -Quiet

63408a6

✨ Add cancellation for DNS lookups

c74d5e7

+ Add GetHostEntryWithCancel method and _cancel member. + Add cancellation for DNS lookups.

vexx32 requested review from adityapatwardhan and daxian-dbw as code owners March 25, 2020 20:20

ghost assigned daxian-dbw Mar 25, 2020

vexx32 added WG-Cmdlets-Management cmdlets in the Microsoft.PowerShell.Management module and removed WG-Cmdlets-Management cmdlets in the Microsoft.PowerShell.Management module labels Mar 25, 2020

PoshChan reviewed Mar 25, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

PoshChan reviewed Mar 25, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

✅ Update tests

9652264

iSazonov reviewed Mar 26, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated Show resolved Hide resolved

iSazonov reviewed Mar 26, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated Show resolved Hide resolved

vexx32 added 2 commits March 26, 2020 08:09

✅ Update test

ea7752a

👌 Address review comments

f947000

iSazonov reviewed Mar 26, 2020

View reviewed changes

Apply suggestions from code review

8b160fe

Co-Authored-By: Ilya <darpa@yandex.ru>

iSazonov reviewed Mar 26, 2020

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1 Outdated Show resolved Hide resolved

Update test/powershell/Modules/Microsoft.PowerShell.Management/Test-C…

e12621a

…onnection.Tests.ps1 Co-Authored-By: Ilya <darpa@yandex.ru>

daxian-dbw reviewed Mar 26, 2020

View reviewed changes

👌 address review comments

bcae912

vexx32 force-pushed the TestConnectionQuietness branch from 322c943 to 0e3665b Compare March 27, 2020 02:52

vexx32 force-pushed the TestConnectionQuietness branch from 0e3665b to 1088377 Compare March 27, 2020 02:58

PoshChan reviewed Mar 27, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

PoshChan reviewed Mar 27, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

PoshChan reviewed Mar 27, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

iSazonov reviewed Mar 27, 2020

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1 Outdated Show resolved Hide resolved

Update test/powershell/Modules/Microsoft.PowerShell.Management/Test-C…

42111ca

…onnection.Tests.ps1 Co-Authored-By: Ilya <darpa@yandex.ru>

iSazonov reviewed Mar 27, 2020

View reviewed changes

Update src/Microsoft.PowerShell.Commands.Management/commands/manageme…

b9a9f26

…nt/TestConnectionCommand.cs Co-Authored-By: Ilya <darpa@yandex.ru>

daxian-dbw approved these changes Mar 28, 2020

View reviewed changes

iSazonov approved these changes Mar 28, 2020

View reviewed changes

iSazonov changed the title ~~Test-Connection: Don't write DNS resolution errors on -Quiet~~ Don't write DNS resolution errors on Test-Connection -Quiet Mar 28, 2020

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Mar 28, 2020

iSazonov added this to the 7.1.0-preview.2 milestone Mar 28, 2020

iSazonov merged commit dfe9955 into PowerShell:master Mar 28, 2020

This was referenced Apr 23, 2020

Test-Connection only responds to stops in between pings #11160

Closed

Test-Connection shows unwanted output when DNS not resolved on Powershell 7.0.0 #12147

Closed

vexx32 deleted the TestConnectionQuietness branch June 13, 2020 22:06


		private static byte[]? s_DefaultSendBuffer;

		private CancellationTokenSource _dnsLookupCancel = new CancellationTokenSource();

	private CancellationTokenSource _dnsLookupCancel = new CancellationTokenSource();
	private readonly CancellationTokenSource _dnsLookupCancel = new CancellationTokenSource();

Don't write DNS resolution errors on Test-Connection -Quiet #12204

Don't write DNS resolution errors on Test-Connection -Quiet #12204

Uh oh!

Conversation

vexx32 commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vexx32 Mar 26, 2020 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

daxian-dbw Mar 27, 2020 • 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!

vexx32 Mar 26, 2020 • 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!

Uh oh!

vexx32 commented Mar 27, 2020

Uh oh!

vexx32 commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vexx32 commented Mar 25, 2020 •

edited

Loading

vexx32 Mar 26, 2020 •

edited

Loading

daxian-dbw Mar 27, 2020 •

edited

Loading

vexx32 Mar 26, 2020 •

edited

Loading

vexx32 commented Mar 27, 2020 •

edited

Loading