Skip to content

Fix FileNotFoundEx crash when Fold happens on untitled: scheme doc #839

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 8, 2019
Merged

Fix FileNotFoundEx crash when Fold happens on untitled: scheme doc #839

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
830c40d
Fix FileNotFoundEx crash when Fold happens on untitled: scheme doc
rkeithhill Jan 1, 2019
ade6c72
Address PR feedback, use TryGetFile() instead of modifying GetFile
rkeithhill Jan 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1376,15 +1376,24 @@ protected Task HandleEvaluateRequest(

#region Event Handlers

private FoldingRange[] Fold(
string documentUri)
private FoldingRange[] Fold(string documentUri)
{
// TODO Should be using dynamic registrations
if (!this.currentSettings.CodeFolding.Enable) { return null; }

// Avoid crash when using untitled: scheme or any other scheme where the document doesn't
// have a backing file. https://github.com/PowerShell/vscode-powershell/issues/1676
// Perhaps a better option would be to parse the contents of the document as a string
// as opposed to reading a file but the senario of "no backing file" probably doesn't
// warrant the extra effort.
ScriptFile scriptFile = editorSession.Workspace.GetFile(documentUri);
Copy link
Member

Choose a reason for hiding this comment

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

I recently added a TryGetFile function that would do the soft check:

PowerShellEditorServices/src/PowerShellEditorServices/Workspace/Workspace.cs

Line 142 in 99f01ef

public bool TryGetFile(string filePath, out ScriptFile scriptFile)

maybe that's better to use here?

Copy link
Contributor Author

@rkeithhill rkeithhill Jan 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We could (perhaps should) change the call site but there are 23 references to GetFile() so maybe we should still try to make GetFile() a little more robust? Either that OR we nuke make GetFile() private and replace all 23 references with calls to TryGetFile(). Longer term, that may be the better strategy because it makes it clearer you may not get a file back. GetFile() behaves similarly except that it returns a null when there is no file to return - which is way less obvious and easier to ignore the null case - kind of like ignoring error return codes in C code. :-)

Copy link
Member

Choose a reason for hiding this comment

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

Actually I don't think GetFile() handle's File exceptions:

PowerShellEditorServices/src/PowerShellEditorServices/Workspace/Workspace.cs

Lines 118 to 119 in 99f01ef

using (FileStream fileStream = new FileStream(resolvedFilePath, FileMode.Open, FileAccess.Read))
using (StreamReader streamReader = new StreamReader(fileStream, Encoding.UTF8))

If FileStream or StreamReader throws an exception, that will bubble up I'm pretty sure, no?

Where as TryGetFile actually does catch them.

I think we should eventually move to using TryGetFile everywhere it makes sense... but we can call that out of scope for this PR if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make the change to use TryGetFile() later tonight. Getting ready to head out to see Aquaman right now. :-)

Copy link
Member

Choose a reason for hiding this comment

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

Have a great time! I want to hear the full @rkeithhill review 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, updated to use TryGetFile() instead - reverted change to GetFile(). Although I did an "important" note on GetFile() that shows up in Intellisense that recommends TryGetFile() instead. Also, fix an incorrect (copy/paste) log message.

P.S. Aquaman was pretty good. Not as good as Wonder Woman IMO but way better than Dawn of Pustice. Then again, I've been a Jason Momoa fan since Stargate Atlantis. :-)

if (scriptFile == null) { return null; }

var result = new List<FoldingRange>();
foreach (FoldingReference fold in TokenOperations.FoldableRegions(
editorSession.Workspace.GetFile(documentUri).ScriptTokens,
this.currentSettings.CodeFolding.ShowLastLine))
FoldingReference[] foldableRegions =
TokenOperations.FoldableRegions(scriptFile.ScriptTokens, this.currentSettings.CodeFolding.ShowLastLine);

foreach (FoldingReference fold in foldableRegions)
{
result.Add(new FoldingRange {
EndCharacter = fold.EndCharacter,
Expand All @@ -1394,6 +1403,7 @@ private FoldingRange[] Fold(
StartLine = fold.StartLine
});
}

return result.ToArray();
}

Expand Down
6 changes: 4 additions & 2 deletions src/PowerShellEditorServices/Workspace/Workspace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,11 @@ public ScriptFile GetFile(string filePath)
string resolvedFilePath = this.ResolveFilePath(filePath);
string keyName = resolvedFilePath.ToLower();

// Make sure the file isn't already loaded into the workspace
// Make sure the file isn't already loaded into the workspace and that the filePath isn't an "in-memory" path.
// There have been crashes caused by this method being called with an "untitled:" document uri. See:
// https://github.com/PowerShell/vscode-powershell/issues/1676
ScriptFile scriptFile = null;
if (!this.workspaceFiles.TryGetValue(keyName, out scriptFile))
if (!this.workspaceFiles.TryGetValue(keyName, out scriptFile) && !IsPathInMemory(filePath))
{
// This method allows FileNotFoundException to bubble up
// if the file isn't found.
Expand Down