-
Notifications
You must be signed in to change notification settings - Fork 7.7k
S.M.A.PowerShell.HadErrors and $? return false positives when errors are suppressed #4613
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
Comments
The whole terminating vs non-terminating errors consistently is misunderstood. Even in some of our product code I see these mistakes and I make them myself. Perhaps an option here is to add HadTerminatingErrors, while HadErrors continues to capture Terminating/Nonterminating (even if handled/suppressed)? |
I agree discussion is required because error handling in PowerShell is a point of confusion. This particular issue has nothing to do with terminating vs non-terminating though. This has to do with handled vs non-handled, which is a completely different thing. If I invoke a .NET method that internally receives some exception but handles that exception gracefully and then eventually returns back to me without error, should $? return $true or $false? The answer in that case is quite clear: it should return $true because there was no error as far as I, the method invoker, am concerned. I believe PowerShell command invocations should behave no differently. In both examples I shared above, I am instructing PowerShell to suppress the error if one occurs. In other words, the error is handled in my command. Yet S.M.A.PowerShell.HadErrors returns Aside -- This related behaviour also surprises me. Try executing these commands one at a time. Get-Service -Name Invalid
$? # returns $false
Get-Service -Name Invalid -ErrorAction Stop
$? # returns $false
try {Get-Service -Name Invalid -ErrorAction Stop} catch {<# Ignore the error, it's benign #>}
$? # returns $false -- shouldn't this return $true?
& {try {Get-Service -Name Invalid -ErrorAction Stop} catch {<# Ignore the error, it's benign #>}}
$? # returns $true |
I see. I think that |
@PowerShell/powershell-committee reviewed this (get-service -name invalid); $?
True
get-service -name invalid; $?
False the two above statements should be equivalent, however, it's not. The bug seems to be that in @KirkMunro 's samples above, #3 should return $true and the two equivalent statements here should both return $false. |
The reason that Re To recap from #3768 (comment):
Similarly (unexpectedly), anything that you invoke via |
Re-reviewing issues @PowerShell/powershell-committee had reviewed prior to GA to see if the decision would still stand post GA. For this issue, based on @mklement0's explanation, it seems this is working as designed and I think just needs to be documented appropriately. |
@SteveL-MSFT I strongly disagree. Suggesting sample 1 from the original post is by design is wrong. You tell PowerShell to outright ignore errors in part of an invocation you make from C#, and then PowerShell says oh, hey, there were errors, so you check, but there weren't any, PowerShell lied. This completely breaks any trust that a developer can have in HadErrors, and forces them to wrap the invocation of PowerShell and build their own HadErrors because HadErrors is broken. You can't document "broken" appropriately. |
@KirkMunro sorry, I focused only on the last few replies. Looking at your original sample, I would tend to agree that if you handle the error (or suppress it by choice), |
@PowerShell/powershell-committee reviewed this:
|
@SteveL-MSFT This statement is not quite right. I think you meant "written to the error stream" not "written to $error". That said, the following code
MUST return false. The $? variable is the only way for a script to determine that a command failed and take action (without adding the complexity of exceptions). For example:
Like any shell, the value of One important thing that is missing is a way for functions and scriptblocks to indicate to the caller that they failed. (Scripts can do it by calling exit 1). |
A few tangents:
True, but as an aside:
That makes sense, but given how easy it is to wipe out the last command's
While an explicit way to do tha
8000
t would definitely be nice, making |
Someone just hit the A recipe such as |
I think perhaps this might be resolved by splitting the functionality expected from Currently it reports a mixed status, depending on the last executed command OR expression. And, as far as I can think this morning, the only way an expression would fail is if it's pretty much nonsensical and is either a parse or something like a math error, using operators in a context they don't allow, e.g., What if we split this in two, so that we have a variable like |
Agreed that
In other words: if an expression (or command) was aborted due to a statement-terminating error, With that, I'd say there's no need for a separate |
That certainly seems to be pretty sensible to me. I can't really think of a case that doesn't effectively cover. 😄 |
I'd amend the quoted statement, as follows: "only ever reflect the status of the last command that was executed in the current scope". I've never, ever wanted it to work differently, and could care less about the status of the last command that was invoked any number of levels deep inside of something I invoked. |
Definitely agreed on that. If my function handles an error state but doesn't itself emit an error, that error should not magically show up in |
Agreed.
& { gci /nosuch }; $? # $True - fortunately, the gci status did NOT leak
& { Write-Error 'oh no' }; $? # !! $True - despite the use of Write-Error The workaround is to use |
This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you. |
1 similar comment
This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you. |
This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you. |
This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes. |
Uh oh!
There was an error while loading. Please reload this page.
Created from discussion started on #3768.
Description
If you invoke PowerShell using the System.Management.Automation.PowerShell class, and if you handle the errors in the command you invoke by indicating that they should be ignored, S.M.A.PowerShell will still set HadErrors to true. Below are some code samples illustrating the problem.
Sample 1: C#/.NET invocation of PowerShell commands/scripts
Sample 1: Expected behavior
$ps.HadErrors should return $false
Sample 1: Actual behavior
$ps.HadErrors returns $true, even when $ps.Streams.Error does not contain any errors (because there were no errors!)
Sample 2: Direct command invocation in PowerShell
Sample 2: Expected behavior
$? should return $true
Sample 2: Actual behavior
$? returns $false, even though there were no errors because the invoker of the command instructed PowerShell to ignore the error that would otherwise have been raised because it is a benign error for them (and therefore, not an error)
Additional Details
In both of these cases, -ErrorAction Ignore is being used to tell PowerShell that the error is actually not an error as far as this invocation is concerned, and therefore it can be completely ignored. Why then, do both $? and$ps.HadErrors indicate there was still error? Technically there was an error, by definition of the command being invoked, but it is being handled/treated like it is not an error in the command from which the command producing the error was invoked. In both of these cases $ ? and $ps.HadErrors should have values indicating that there wasn't an error. If you do not trust the command you are invoking to ignore benign errors and only notify you about errors that you actually need to care about then you should not be invoking that command.
Environment data
The text was updated successfully, but these errors were encountered: