Don't lock Cake host object assembly. #1044
Conversation
…ject type. This avoids locking file in workspace.
|
|
||
| namespace OmniSharp.Services | ||
| { | ||
| internal class AssemblyLoader : IAssemblyLoader | ||
| { | ||
| private static readonly ConcurrentDictionary<string, Assembly> AssemblyCache = new ConcurrentDictionary<string, Assembly>( | ||
| PlatformHelper.IsWindows ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal); |
There was a problem hiding this comment.
Note that OSX is also case-insensitive. Honestly, I'm not sure the platform check is worth it. In general, we usually just compare file paths case-insensitively in OmniSharp -- even on Linux. The number of collisions we've had with this in practice is zero. 😄
There was a problem hiding this comment.
Ok, I'll update it to a StringComparer.OrdinalIgnoreCase then.
There was a problem hiding this comment.
Additionally, wan't the cache to cover all methods and not only when loading from byte[]?
| { | ||
| _logger.LogError(ex, $"Failed to load assembly from path: {assemblyPath}"); | ||
| } | ||
| if (assembly != null) |
There was a problem hiding this comment.
tiny nit: I'd put a line break before to visually separate the if statement from the catch handler.
| @@ -77,5 +81,31 @@ public Assembly LoadFrom(string assemblyPath) | |||
| _logger.LogTrace($"Assembly loaded from path: {assemblyPath}"); | |||
| return assembly; | |||
| } | |||
|
|
|||
| public Assembly LoadFrom(string assemblyPath, bool lockAssembly) | |||
There was a problem hiding this comment.
Should the two LoadFrom overloads be merged with lockAssembly becoming an optional parameter? Also, from an API perspective, I'd sort of prefer to see dontLockAssemblyOnDisk since the goal in calling this method is to avoid locking.
There was a problem hiding this comment.
I could merge if you like. And yeah, the naming dontLockAssemblyOnDisk sounds better, with a default to false.
- Rename optional parameter to dontLockAssemblyOnDisk
Don't lock Cake host object assembly (Cake.Core) when loading host object type. We'd like to avoid locking file in workspace.