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 file close in workspace service for Linux #1902

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

fflaten
Copy link
Contributor

@fflaten fflaten commented Aug 18, 2022

PR Summary

Use a consistent dictionary key for CRUD operations in WorkspaceService.workspaceFiles

PR Context

Fix #1901

@fflaten fflaten requested a review from a team August 18, 2022 17:54
Copy link
Member

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

Hahaha, I remember seeing this bug and just forgetting to fix it. Thank you so much 😄

@fflaten
Copy link
Contributor Author

fflaten commented Aug 18, 2022

Should ScriptFile.Id maybe just be a guid?

/// <summary>
/// Gets a unique string that identifies this file. At this time,
/// this property returns a normalized version of the value stored
/// in the FilePath property.
/// </summary>
public string Id => FilePath.ToLower();

Looks like it's only used internally atm which could use the same helper, but I guess we'd want a public Id for future use. Could possibly cause conflicts on Linux atm.

public ScriptFile[] ExpandScriptReferences(ScriptFile scriptFile)
{
Dictionary<string, ScriptFile> referencedScriptFiles = new();
List<ScriptFile> expandedReferences = new();
// add original file so it's not searched for, then find all file references
referencedScriptFiles.Add(scriptFile.Id, scriptFile);
RecursivelyFindReferences(scriptFile, referencedScriptFiles);
// remove original file from referenced file and add it as the first element of the
// expanded referenced list to maintain order so the original file is always first in the list
referencedScriptFiles.Remove(scriptFile.Id);
expandedReferences.Add(scriptFile);
if (referencedScriptFiles.Count > 0)
{
expandedReferences.AddRange(referencedScriptFiles.Values);
}
return expandedReferences.ToArray();
}

@andyleejordan andyleejordan enabled auto-merge (squash) August 18, 2022 17:58
@fflaten
Copy link
Contributor Author

fflaten commented Aug 18, 2022

Hahaha, I remember seeing this bug and just forgetting to fix it. Thank you so much 😄

Initially hoped it would improve symbol search perf by not keeping to many files opened in the service, but it won't. Apparently there are many ways files get opened but only closing them in editor will trigger CloseFile() to remove them from service. So now it's just a bugfix 😄

@andyleejordan
Copy link
Member

I'm getting tempted to skip DebuggerSetsVariablesNoConversion as it's also being really flaky.

@andyleejordan andyleejordan merged commit 75111b6 into PowerShell:main Aug 18, 2022
@fflaten fflaten deleted the linux-fileclose branch August 30, 2022 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Files are not closed in workspace service in Linux
2 participants