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

Fix null dereference when debugging untitlted files #725

Merged
merged 2 commits into from
Aug 20, 2018

Conversation

rjmholt
Copy link
Contributor

@rjmholt rjmholt commented Aug 16, 2018

Fixes PowerShell/vscode-powershell#1334.

I was looking at this trying to figure out what's going on, and I think what's happening is that the null check we do it not helpful because we don't await the execution of the method.

So I've changed it to pass the actual value of the string in at call time. I assume that will work, but let me know if not.

Also the first commit here was a style change one...

var nonAwaitedTask =
this.LaunchScript(requestContext)
.ConfigureAwait(false);
var nonAwaitedTask = LaunchScript(requestContext, _scriptToLaunch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the real change

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM - I'm not too sure bit that that's the problem... but you've made the function more functional so I'm happy about that 👍

@rjmholt
Copy link
Contributor Author

rjmholt commented Aug 17, 2018

Well there's a null check around the call, but I assume the object field is changed after the check but before the evaluation.

Here's the tail of the logs from the issue:

2018-05-25 11:22:25 [ERROR] - Method "OnListenTaskCompleted" at line 391 of C:\projects\powershelleditorservices\src\PowerShellEditorServices.Protocol\MessageProtocol\ProtocolEndpoint.cs

    ProtocolEndpoint message loop terminated due to unhandled exception:
    
    System.AggregateException: One or more errors occurred. ---> System.ArgumentNullException: Value cannot be null.
    Parameter name: path
       at Microsoft.PowerShell.EditorServices.ScriptFile.IsUntitledPath(String path)
       at Microsoft.PowerShell.EditorServices.Protocol.Server.DebugAdapter.<HandleLaunchRequest>d__26.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.MessageDispatcher.<DispatchMessage>d__7.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.ProtocolEndpoint.<ListenForMessages>d__36.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Utility.AsyncContext.Start(Func`1 asyncMainFunc, ILogger logger)
       at System.Threading.Tasks.Task.Execute()
       --- End of inner exception stack trace ---
    ---> (Inner Exception #0) System.ArgumentNullException: Value cannot be null.
    Parameter name: path
       at Microsoft.PowerShell.EditorServices.ScriptFile.IsUntitledPath(String path)
       at Microsoft.PowerShell.EditorServices.Protocol.Server.DebugAdapter.<HandleLaunchRequest>d__26.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.MessageDispatcher.<DispatchMessage>d__7.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.ProtocolEndpoint.<ListenForMessages>d__36.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Utility.AsyncContext.Start(Func`1 asyncMainFunc, ILogger logger)
       at System.Threading.Tasks.Task.Execute()<---
    

2018-05-25 11:22:25 [ERROR] - Method "ProtocolEndpoint_UnhandledException" at line 434 of C:\projects\powershelleditorservices\src\PowerShellEditorServices.Host\EditorServicesHost.cs

    PowerShell Editor Services is terminating due to an unhandled exception, see previous logs for details.

@TylerLeonhardt
Copy link
Member

Makes sense :)

@rjmholt
Copy link
Contributor Author

rjmholt commented Aug 17, 2018

To be honest I get the impression there are more than a few race conditions hanging around the codebase and sharing object fields over tasks is responsible. So a more functional style might be our saviour here

@TylerLeonhardt
Copy link
Member

PURE FUNCTIONS PLEASE

@rjmholt
Copy link
Contributor Author

rjmholt commented Aug 17, 2018

[Pure]

Copy link
Contributor

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM

+1 to there being a lot of race conditions. There's a lot of race condition fixes in the PSRL changes, but that was mostly through adding locks. Those areas could probably benefit from similar refactorings, the locks were more like stop gaps. Nice work!

@TylerLeonhardt TylerLeonhardt merged commit f1ab462 into PowerShell:master Aug 20, 2018
@rjmholt rjmholt deleted the debug-null-ref-fix branch December 12, 2018 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants