Error handling when loading assemblies from an external folder #866
Conversation
filipw
force-pushed
the
bugfix/extensions-non-existing-path
branch
from
ac8a22f
to
0cbaf04
Compare
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, $"An error occurred when attempting to access '{folderPath}'."); |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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
| if (assembly != null) | ||
| var assemblies = new List<Assembly>(); | ||
|
|
||
| foreach (var filePath in Directory.EnumerateFiles(folderPath, "*.dll")) |
There was a problem hiding this comment.
maybe this should be SearchOption.AllDirectories?
There was a problem hiding this comment.
I'd rather that be passed in. Could be surprising behavior.
There was a problem hiding this comment.
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)
Follow up to #848
Added try/catch to the directory enumeration inside
AssemblyLoader. It can throw due to a number of problems - directory not found, path too long, lack of permissions etc.We should gracefully exit with no assemblies loaded and not crash OmniSharp.