Make the controller and repository stack async #28
Conversation
| { | ||
| return (Get(options)).FormattedAsCountResult<ODataPackage>(); | ||
| return (await Get(options, token)).FormattedAsCountResult<ODataPackage>(); |
There was a problem hiding this comment.
I considered this, but I wanted to leave the controller actions with their existing names. There is mapping logic around the $count methods that would make changing these action names a little nasty. So I thought the cost outweighed the benefit.
| { | ||
| try | ||
| // Immediatelly defer work to the background thread. |
|
|
||
| return true; | ||
| private Task<ServerPackage> CreateServerPackageAsync( |
There was a problem hiding this comment.
I actually used this method as a forcing factor to make all callers async. I picked this location since it is most likely to invoke async methods in the future 😉. However, with the Task.Yield() in TryAddServerPackageAsync leaves the public API the same no matter what is done in this private method. So, sure.
| @@ -362,61 +373,73 @@ protected virtual void Dispose(bool disposing) | |||
| _serverPackageCache.PersistIfDirty(); | |||
| } | |||
|
|
|||
| private void RebuildPackageStore() | |||
| private async void RebuildPackageStoreAsync(CancellationToken token) | |||
There was a problem hiding this comment.
If an exception occurs in this handler, the process will terminate. It seems like we should have some recovery and/or logging here.
There was a problem hiding this comment.
Nice catch! I didn't know fire-and-forget tasks can do this.
There was a problem hiding this comment.
Please verify the runtime behavior. I may be wrong.
There was a problem hiding this comment.
Just did. You're totally right. IMO this is a bit unintuitive!
| _fileSystemWatcher.Dispose(); | ||
| _fileSystemWatcher = null; | ||
|
|
||
| _logger.Log(LogLevel.Verbose, "Destroyed FileSystemWatcher - no longer monitoring {0}.", Source); | ||
| } | ||
| } | ||
|
|
||
| private void FileSystemChanged(object sender, FileSystemEventArgs e) | ||
| private async void FileSystemChangedAsync(object sender, FileSystemEventArgs e) |
There was a problem hiding this comment.
An exception from this method would terminate the process.
| { | ||
| _logger.Log(LogLevel.Error, "Error while reading packages from disk: {0} {1}", ex.Message, ex.StackTrace); | ||
| _logger.Log(LogLevel.Info, "Finished reading packages from disk."); | ||
| ; |
| return handle; | ||
| } | ||
|
|
||
| private class DisposableSemphoreSlim : IDisposable |
There was a problem hiding this comment.
Semphore -> Semaphore
SemaphoreSlim already implements IDisposable, which makes this name a little nonsensical.
How about renaming to Lock?
There was a problem hiding this comment.
Sounds fine. The intend is that Dispose releases the semaphore instead of what Semaphore.Dispose does.
| _semaphore.Release(); | ||
| _lockTaken = false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private class SupressedFileSystemWatcher : IDisposable |
| } | ||
|
|
||
| /// <summary> | ||
| /// ReadPackagesFromDisk loads all packages from disk and determines additional metadata such as the hash, IsAbsoluteLatestVersion, and IsLatestVersion. | ||
| /// ReadPackagesFromDisk loads all packages from disk and determines additional metadata such as the hash, |
|
|
||
| foreach (var packageFile in _fileSystem.GetFiles(_fileSystem.Root, "*.nupkg", false)) | ||
| foreach (var packageFile in _fileSystem.GetFiles(_fileSystem.Root, "*.nupkg", false)) |
| private ServerPackage CreateServerPackage( | ||
| IPackage package, | ||
| bool enableDelisting, | ||
| CancellationToken token) |
There was a problem hiding this comment.
Good point. I'll fix this in a subsequent PR (will have one soon for SemVer 2.0.0 which is in master but not dev).
To facilitate async IO operations at the bottom of the stack, I have changed
IServerPackageRepository,IServerPackageStore, andNuGetODataControllerto be async.This will ease future improvements such as switching from NuGet.Core (sync APIs) to NuGet 3.x/4.x (a lot of async APIs).
/cc @dtivel @emgarten @maartenba