Skip to content

Commit

Permalink
Merge pull request #180 from pjf/179_file_locking
Browse files Browse the repository at this point in the history 
Wrap many disposable resources in `using` blocks.
  • Loading branch information
@AlexanderDzhoganov
AlexanderDzhoganov committed Oct 27, 2014
2 parents ba91ba7 + 374af33 commit 7dfee7d
Show file tree
Hide file tree
Showing 16 changed files with 409 additions and 176 deletions.
8 changes: 4 additions & 4 deletions CKAN/CKAN/CKAN.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,8 @@
<Compile Include="Properties\AssemblyInfo.cs" />
<Compile Include="Module.cs" />
<Compile Include="KSP.cs" />
<Compile Include="InstalledModule.cs" />
<Compile Include="RegistryManager.cs" />
<Compile Include="Registry.cs" />
<Compile Include="ModuleInstaller.cs" />
<Compile Include="Net.cs" />
<Compile Include="AvailableModule.cs" />
<Compile Include="User.cs" />
<Compile Include="RelationshipResolver.cs" />
<Compile Include="Types\Version.cs" />
Expand All @@ -75,6 +71,10 @@
<Compile Include="NetAsyncDownloader.cs" />
<Compile Include="Cache.cs" />
<Compile Include="FilesystemTransaction.cs" />
<Compile Include="Registry\AvailableModule.cs" />
<Compile Include="Registry\InstalledModule.cs" />
<Compile Include="Registry\Registry.cs" />
<Compile Include="Registry\RegistryManager.cs" />
</ItemGroup>
<Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
<ItemGroup>
Expand Down
6 changes: 6 additions & 0 deletions CKAN/CKAN/FilesystemTransaction.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,12 @@ public void Snapshot(string path)
fileManager.Snapshot(path);
}

/// <summary>
/// Opens a file for writing as part of this transaction.
/// The caller is responsible for closing and disposing of the filehandle as appropraite.
/// </summary>

// TODO: neverOverwrite seems to be unused here, remove it.
public FileStream OpenFileWrite(string path, bool neverOverwrite = true)
{
if (TempFiles.ContainsKey(path))
Expand Down
169 changes: 103 additions & 66 deletions CKAN/CKAN/ModuleInstaller.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,22 +335,23 @@ public List<string> GetModuleContentsList(CkanModule module)

/// <summary>
/// Install our mod from the filename supplied.
/// If no file is supplied, we will fetch() it first.
/// If no file is supplied, we will check the cache or download it.
/// Does *not* resolve dependencies; this actually does the heavy listing.
/// Use InstallList() for requests from the user.
///
/// XXX: This provides no way to check if the install failed,
/// it *should* throw an exception if it does.
/// </summary>
private void Install(CkanModule module, string filename = null)
//
// TODO: The name of this and InstallModule() need to be made more distinctive.

internal void Install(CkanModule module, string filename = null)
{
User.WriteLine(module.identifier + ":\n");

Version version = registry_manager.registry.InstalledVersion(module.identifier);

// TODO: This really should be handled by higher-up code.
if (version != null)
{
// TODO: Check if we can upgrade!
User.WriteLine(" {0} {1} already installed, skipped", module.identifier, version);
return;
}
Expand All @@ -367,44 +368,15 @@ private void Install(CkanModule module, string filename = null)
// And a list of files to record them to.
var module_files = new Dictionary<string, InstalledModuleFile>();

ZipFile zipfile = null;

// Open our zip file for processing
try
{
zipfile = new ZipFile(File.OpenRead(filename));
}
catch (Exception)
{
// TODO: I'm not sure we want to just be returing here
// on error. A failed install is enough of a reason to
// bail out entirely. This should be throwing an exception.
User.Error("Failed to open archive \"{0}\"", filename);
return;
}

if (module.install == null || module.install.Length == 0)
{
log.DebugFormat("No install stanzas found for {0}, using defaults", module);

// This throws a FileNotFoundKraken on failure. We intentionally
// don't catch it, because that's an irrecoverable error.
var stanza = GenerateDefaultInstall(module.identifier, zipfile);
InstallComponent(stanza, zipfile, module_files);
}
else
{
// Walk through our install instructions.
foreach (ModuleInstallDescriptor stanza in module.install)
{
InstallComponent(stanza, zipfile, module_files);
}
}
// Install all the things!
InstallModule(module, filename, module_files);

// Register our files.
registry.RegisterModule(new InstalledModule(module_files, module, DateTime.Now));

// Handle bundled mods, if we have them.
// TODO: Deprecate bundles!

if (module.bundles != null)
{
foreach (BundledModuleDescriptor stanza in module.bundles)
Expand All @@ -425,13 +397,16 @@ private void Install(CkanModule module, string filename = null)
// Not installed, so let's get about installing it!
var installed_files = new Dictionary<string, InstalledModuleFile>();

InstallComponent(stanza, zipfile, installed_files);
InstallComponent(stanza, filename, installed_files);

registry.RegisterModule(new InstalledModule(installed_files, bundled, DateTime.Now));
}
}

// Done! Save our registry changes!
// TODO: From a transaction standpoint, we probably don't want to save the registry,
// and instead want something higher up the call-chain to do that.

registry_manager.Save();

if (onReportModInstalled != null)
Expand All @@ -451,7 +426,12 @@ private string Sha1Sum(string path)

try
{
return BitConverter.ToString(hasher.ComputeHash(File.OpenRead(path)));
using (var fh = File.OpenRead(path))
{
string sha1 = BitConverter.ToString(hasher.ComputeHash(fh));
fh.Close();
return sha1;
}
}
catch
{
Expand Down Expand Up @@ -513,23 +493,56 @@ internal static ModuleInstallDescriptor GenerateDefaultInstall(string identifier
/// <summary>
/// Install the component described in the stanza.
/// Modifies the supplied module_files to contain the files installed.
/// This method should be avoided, as it may be removed in the future.
/// </summary>
private void InstallComponent(InstallableDescriptor stanza, ZipFile zipfile,
internal void InstallComponent(InstallableDescriptor stanza, string zip_filename,
Dictionary<string, InstalledModuleFile> module_files)
{
List<InstallableFile> files = FindInstallableFiles(stanza, zipfile, ksp);

foreach (var file in files) {
// TODO: Can we deprecate this code now please?
log.Warn("Soon to be deprecated method InstallComponent called");

User.WriteLine(" * Copying " + file.source.Name);
using (ZipFile zipfile = new ZipFile(zip_filename))
{
List<InstallableFile> files = FindInstallableFiles(stanza, zipfile, ksp);

foreach (var file in files) {

// TODO: I'd like to replace this with a log.Info
User.WriteLine(" * Copying " + file.source.Name);

CopyZipEntry(zipfile, file.source, file.destination, file.makedir);

// TODO: We really should be computing sha1sums again!
module_files.Add(file.destination, new InstalledModuleFile
{
sha1_sum = "" //Sha1Sum (currentTransaction.OpenFile(fullPath).TemporaryPath)
});
}
}
}

CopyZipEntry(zipfile, file.source, file.destination, file.makedir);
/// <summary>
/// Installs the module from the zipfile provided, updating the supplied list of installed files provided.
///
/// Propagates up a BadMetadataKraken if our install metadata is bad.
/// </summary>
internal void InstallModule(CkanModule module, string zip_filename, Dictionary<string, InstalledModuleFile> installed_files)
{
using (ZipFile zipfile = new ZipFile(zip_filename))
{
List<InstallableFile> files = FindInstallableFiles(module, zipfile, ksp);

// TODO: We really should be computing sha1sums again!
module_files.Add(file.destination, new InstalledModuleFile
foreach (var file in files)
{
sha1_sum = "" //Sha1Sum (currentTransaction.OpenFile(fullPath).TemporaryPath)
});
log.InfoFormat("Copying {0}", file.source.Name);
CopyZipEntry(zipfile,file.source,file.destination,file.makedir);
installed_files.Add(file.destination, new InstalledModuleFile
{
// TODO: Re-enable checksums!!!
sha1_sum = "" //Sha1Sum (currentTransaction.OpenFile(fullPath).TemporaryPath)
});
}
}
}

Expand All @@ -541,6 +554,8 @@ private void InstallComponent(InstallableDescriptor stanza, ZipFile zipfile,
///
/// Throws a BadInstallLocationKraken if the install stanza targets an
/// unknown install location (eg: not GameData, Ships, etc)
///
/// Throws a BadMetadataKraken if the stanza resulted in no files being returned.
/// </summary>
internal static List<InstallableFile> FindInstallableFiles(InstallableDescriptor stanza, ZipFile zipfile, KSP ksp)
{
Expand Down Expand Up @@ -621,6 +636,13 @@ internal static List<InstallableFile> FindInstallableFiles(InstallableDescriptor
files.Add(file_info);
}

// If we have no files, then something is wrong! (KSP-CKAN/CKAN#93)
if (files.Count == 0)
{
// We have null as the first argument here, because we don't know which module we're installing
throw new BadMetadataKraken(null, String.Format("No files found in {0} to install!", stanza.file));
}

return files;
}

Expand All @@ -629,23 +651,35 @@ internal static List<InstallableFile> FindInstallableFiles(InstallableDescriptor
/// for this module.
///
/// If a KSP instance is provided, it will be used to generate output paths, otherwise these will be null.
///
/// Throws a BadMetadataKraken if the stanza resulted in no files being returned.
/// </summary>
internal static List<InstallableFile> FindInstallableFiles(CkanModule module, ZipFile zipfile, KSP ksp)
{
var files = new List<InstallableFile> ();

// Use the provided stanzas, or use the default install stanza if they're absent.
if (module.install != null && module.install.Length != 0)
try
{
foreach (ModuleInstallDescriptor stanza in module.install)
// Use the provided stanzas, or use the default install stanza if they're absent.
if (module.install != null && module.install.Length != 0)
{
foreach (ModuleInstallDescriptor stanza in module.install)
{
files.AddRange(FindInstallableFiles(stanza, zipfile, ksp));
}
}
else
{
files.AddRange(FindInstallableFiles(stanza, zipfile, ksp));
ModuleInstallDescriptor default_stanza = GenerateDefaultInstall(module.identifier, zipfile);
files.AddRange(FindInstallableFiles(default_stanza, zipfile, ksp));
}
}
else
catch (BadMetadataKraken kraken)
{
ModuleInstallDescriptor default_stanza = GenerateDefaultInstall(module.identifier, zipfile);
files.AddRange(FindInstallableFiles(default_stanza, zipfile, ksp));
// Decorate our kraken with the current module, as the lower-level
// methods won't know it.
kraken.module = module;
throw;
}

return files;
Expand All @@ -657,13 +691,15 @@ internal static List<InstallableFile> FindInstallableFiles(CkanModule module, Zi
///
/// This *will* throw an exception if the file does not exist.
///
/// Throws a BadMetadataKraken if the stanza resulted in no files being returned.
///
/// If a KSP instance is provided, it will be used to generate output paths, otherwise these will be null.
/// </summary>
// TODO: Document which exception!
internal static List<InstallableFile> FindInstallableFiles(CkanModule module, string zip_filename, KSP ksp)
{
// `using` makes sure our zipfile gets closed when we exit this block.
using (ZipFile zipfile = new ZipFile(File.OpenRead(zip_filename)))
using (ZipFile zipfile = new ZipFile(zip_filename))
{
log.DebugFormat("Searching {0} using {1} as module", zip_filename, module);
return FindInstallableFiles(module, zipfile, ksp);
Expand Down Expand Up @@ -697,16 +733,17 @@ private void CopyZipEntry(ZipFile zipfile, ZipEntry entry, string fullPath, bool
}

// It's a file! Prepare the streams
Stream zipStream = zipfile.GetInputStream(entry);

FileStream output = currentTransaction.OpenFileWrite(fullPath);

// Copy
zipStream.CopyTo(output);
using (Stream zipStream = zipfile.GetInputStream(entry))
using (FileStream output = currentTransaction.OpenFileWrite(fullPath))
{
// Copy
zipStream.CopyTo(output);

// Tidy up.
zipStream.Close();
output.Close();
// Tidy up. Possibly not needed inside a `using` block, but almost certainly
// doesn't hurt.
zipStream.Close();
output.Close();
}
}

return;
Expand Down
1 change: 0 additions & 1 deletion CKAN/CKAN/NetAsyncDownloader.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ public struct NetAsyncDownloaderDownloadPart
public long bytesLeft;
public int bytesPerSecond;
public Exception error;
public FileStream fileStream;
public int lastProgressUpdateSize;
public DateTime lastProgressUpdateTime;
public string path;
Expand Down
0 CKAN/CKAN/AvailableModule.cs → CKAN/CKAN/Registry/AvailableModule.cs
View file
File renamed without changes.
6 changes: 6 additions & 0 deletions CKAN/CKAN/InstalledModule.cs → CKAN/CKAN/Registry/InstalledModule.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ public class InstalledModuleFile
public string sha1_sum;
}

/// <summary>
/// A simple clss that represents an installed module. Includes the time of installation,
/// the module itself, and a list of files installed with it.
///
/// Primarily used by the Registry class.
/// </summary>
public class InstalledModule
{
public DateTime install_time;
Expand Down
Loading

0 comments on commit 7dfee7d

Please sign in to comment.