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

Error handling when loading assemblies from an external folder #866

Merged
merged 4 commits into from
May 25, 2017
Merged

Error handling when loading assemblies from an external folder #866

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
76906f9
check if folder path exists before attempting to load assemblies
filipw May 18, 2017
0cbaf04
extra try/catch
filipw May 18, 2017
18b16dd
Merge branch 'dev' into bugfix/extensions-non-existing-path
filipw May 20, 2017
8ffc3c8
Merge branch 'dev' into bugfix/extensions-non-existing-path
DustinCampbell May 25, 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
23 changes: 16 additions & 7 deletions src/OmniSharp.Host/Services/AssemblyLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,26 @@ public IReadOnlyList<Assembly> LoadAllFrom(string folderPath)
{
if (string.IsNullOrWhiteSpace(folderPath)) return Array.Empty<Assembly>();

var assemblies = new List<Assembly>();
foreach (var filePath in Directory.EnumerateFiles(folderPath, "*.dll"))
try
{
var assembly = LoadFromPath(filePath);
if (assembly != null)
var assemblies = new List<Assembly>();

foreach (var filePath in Directory.EnumerateFiles(folderPath, "*.dll"))
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe this should be SearchOption.AllDirectories?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather that be passed in. Could be surprising behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah maybe it's too much at all to even think about it. let's keep it simple for now and when/if more advanced use cases come up then we can decide what to do (i.e. allow this setting to be bound from config too and so on)

{
assemblies.Add(assembly);
var assembly = LoadFromPath(filePath);
if (assembly != null)
{
assemblies.Add(assembly);
}
}
}

return assemblies;
return assemblies;
}
catch (Exception ex)
{
_logger.LogError(ex, $"An error occurred when attempting to access '{folderPath}'.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This message could be wrong if something else fails. I can't see what would fail though. 😄

Is there any reason not to check Directory.Exists(folderPath)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had it in the initial commit (76906f9) but then I decided to go for try/catch instead as that would cover a lot more potential edge cases i.e. PathTooLongException, SecurityException, UnauthorizedAccessException, IOException rather than just DirectoryNotFoundException

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

return Array.Empty<Assembly>();
}
}

private Assembly LoadFromPath(string assemblyPath)
Expand Down