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.

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

Renamed documents should be removed from the workspace and not cached #783

Closed
wants to merge 2 commits into from
Closed

Renamed documents should be removed from the workspace and not cached #783

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7da4ad8
Renamed documents should be removed from the workspace and not cached
Feb 27, 2017
c197884
Merge branch 'dev' into bugfix/fix_caching_of_file_after_rename
eByte23 Jun 9, 2017
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
36 changes: 35 additions & 1 deletion src/OmniSharp.Roslyn.CSharp/Services/Files/OnFilesChangedService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,66 @@
using System.Collections.Generic;
using System.Composition;
using System.IO;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using OmniSharp.FileWatching;
using OmniSharp.Mef;
using OmniSharp.Models;
using OmniSharp.Models.FilesChanged;


Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary blank line.

namespace OmniSharp.Roslyn.CSharp.Services.Files
{
[OmniSharpHandler(OmniSharpEndpoints.FilesChanged, LanguageNames.CSharp)]
public class OnFilesChangedService : IRequestHandler<IEnumerable<Request>, FilesChangedResponse>
{
private readonly IFileSystemWatcher _watcher;
private OmniSharpWorkspace _workspace;
private object _lock = new object();
Copy link
Contributor

Choose a reason for hiding this comment

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

these fields should be readonly


[ImportingConstructor]
public OnFilesChangedService(IFileSystemWatcher watcher)
public OnFilesChangedService(IFileSystemWatcher watcher, OmniSharpWorkspace workspace)
{
_watcher = watcher;
_workspace = workspace;
}

public Task<FilesChangedResponse> Handle(IEnumerable<Request> requests)
{
foreach (var request in requests)
{
RemoveRenamedFiles(request.FileName);
_watcher.TriggerChange(request.FileName);
}
return Task.FromResult(new FilesChangedResponse());
}

private void RemoveRenamedFiles(string fileName)
{
lock (_lock)
{
var path = Path.GetDirectoryName(fileName);
foreach (var project in _workspace.CurrentSolution.Projects)
{
if (!path.StartsWith(Path.GetDirectoryName(project.FilePath)))
Copy link
Contributor

Choose a reason for hiding this comment

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

StringComparison.OrdinalIgnoreCase

{
continue;
}

foreach (var document in project.Documents)
{
if (!document.FilePath.Contains(path))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be Equals, not Contains. I don't want to accidentally remove Foo.xaml.cs when Foo.xaml is removed. Also, StringComparison.OrdinalIgnoreCase.

{
continue;
}

if(!File.Exists(document.FilePath) && _workspace.CurrentSolution.ContainsDocument(document.Id))
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change the loop to stop accessing _workspace.CurrentSolution all of the time, the call to ContainsDocument is unnecessary.

{
_workspace.TryApplyChanges(_workspace.CurrentSolution.RemoveDocument(document.Id));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the wrong approach and can possible fail if multiple documents are removed from different projects. The reason for this is because _worksapce.TryApplyChanges(...) will update _workspace.CurrentSolution. However, the foreach loop will have received an enumerator from the original Solution.

I recommend the following approach:

  1. Capture _workspace.CurrentSolution into a local variable.
  2. Loop through all its projects and documents and make necessary changes to a new Solution.
  3. After the loop, call _workpsace.TryApplyChanges with the new Solution if it is different than the old Solution.

Because all of this is done on immutable data structures (except the workspace itself), you shouldn't need the lock at all. Instead, it should be something like this (modulo errors because I'm tying into GitHub):

var directoryName = Path.GetDirectoryName(fileName);

var oldSolution = _workspace.CurrentSolution;
var newSolution = oldSolution;

foreach (var project in oldSolution.Projects)
{
    if (!directoryName.StartsWith(Path.GetDirectoryName(project.FilePath), StringComparison.OrdinalIgnoreCase))
    {
        continue;
    }

    foreach (var document in project.Documents)
    {
        if (fileName.Equals(document.FilePath, StringComparison.OrdinalIgnoreCase) &&
            !File.Exists(fileName))
        {
            newSolution = newSolution.RemoveDocument(document.Id);
        }
    }
}

if (oldSolution != newSolution)
{
   _workspace.TryApplyChanges(newSolution);
}

}
}
}
}
}
}
}