Skip to content

Commit 615b333

Browse files
rmarinhoCopilot
andcommitted
Address PR #274 feedback: improve JdkInstaller robustness
- Fix GetStringAsync to use GetAsync + ReadAsStringAsync for cancellation support - Improve tar argument escaping using single quotes for paths with special chars - Implement rollback logic: restore previous JDK if move fails - Document .NET Standard 2.0 ZipFile cancellation limitation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent c89df41 commit 615b333

1 file changed

Lines changed: 44 additions & 11 deletions

File tree

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs

Lines changed: 44 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,9 @@ public async Task InstallAsync (int majorVersion, string targetPath, IProgress<J
219219

220220
// Try to fetch checksum
221221
try {
222-
var checksumContent = await httpClient.GetStringAsync (versionInfo.ChecksumUrl).ConfigureAwait (false);
222+
using var checksumResponse = await httpClient.GetAsync (versionInfo.ChecksumUrl, cancellationToken).ConfigureAwait (false);
223+
checksumResponse.EnsureSuccessStatusCode ();
224+
var checksumContent = await checksumResponse.Content.ReadAsStringAsync ().ConfigureAwait (false);
223225
versionInfo.Checksum = ParseChecksumFile (checksumContent);
224226
logger (TraceLevel.Verbose, $"Checksum for JDK {majorVersion}: {versionInfo.Checksum}");
225227
}
@@ -465,6 +467,10 @@ async Task ExtractArchiveAsync (string archivePath, string targetPath, Cancellat
465467
Directory.CreateDirectory (tempExtractPath);
466468

467469
if (OS.IsWindows) {
470+
// Note: ZipFile.ExtractToDirectory in .NET Standard 2.0 does not support cancellation tokens.
471+
// This is a .NET API limitation. The tar extraction on Unix/macOS does support cancellation.
472+
// For very large archives, consider implementing entry-by-entry extraction if cancellation
473+
// responsiveness on Windows becomes critical.
468474
ZipFile.ExtractToDirectory (archivePath, tempExtractPath);
469475
}
470476
else {
@@ -483,23 +489,46 @@ async Task ExtractArchiveAsync (string archivePath, string targetPath, Cancellat
483489
jdkRoot = contentsHome;
484490
}
485491

486-
// Move to final destination (atomic swap to avoid Windows race condition)
492+
// Move to final destination with rollback on failure
493+
string? backupPath = null;
487494
if (Directory.Exists (targetPath)) {
488-
var backupPath = targetPath + $".old-{Guid.NewGuid ():N}";
495+
backupPath = targetPath + $".old-{Guid.NewGuid ():N}";
489496
Directory.Move (targetPath, backupPath);
490-
try {
491-
Directory.Delete (backupPath, recursive: true);
492-
}
493-
catch (Exception ex) {
494-
logger (TraceLevel.Warning, $"Could not clean up old JDK at '{backupPath}': {ex.Message}");
495-
}
496497
}
497498

498499
var parentDir = Path.GetDirectoryName (targetPath);
499500
if (!string.IsNullOrEmpty (parentDir))
500501
Directory.CreateDirectory (parentDir);
501502

502-
Directory.Move (jdkRoot, targetPath);
503+
try {
504+
Directory.Move (jdkRoot, targetPath);
505+
506+
// Only delete backup after successful move
507+
if (backupPath != null && Directory.Exists (backupPath)) {
508+
try {
509+
Directory.Delete (backupPath, recursive: true);
510+
}
511+
catch (Exception ex) {
512+
logger (TraceLevel.Warning, $"Could not clean up old JDK at '{backupPath}': {ex.Message}");
513+
}
514+
}
515+
}
516+
catch (Exception ex) {
517+
// Rollback: restore the original JDK from backup
518+
logger (TraceLevel.Error, $"Failed to move JDK to '{targetPath}': {ex.Message}");
519+
if (backupPath != null && Directory.Exists (backupPath)) {
520+
try {
521+
if (Directory.Exists (targetPath))
522+
Directory.Delete (targetPath, recursive: true);
523+
Directory.Move (backupPath, targetPath);
524+
logger (TraceLevel.Warning, $"Restored previous JDK from backup '{backupPath}'.");
525+
}
526+
catch (Exception restoreEx) {
527+
logger (TraceLevel.Error, $"Failed to restore previous JDK from backup '{backupPath}': {restoreEx.Message}");
528+
}
529+
}
530+
throw;
531+
}
503532
}
504533
finally {
505534
if (Directory.Exists (tempExtractPath))
@@ -511,9 +540,13 @@ async Task ExtractArchiveAsync (string archivePath, string targetPath, Cancellat
511540

512541
async Task ExtractTarGzAsync (string archivePath, string destinationPath, CancellationToken cancellationToken)
513542
{
543+
// Use single quotes to properly handle paths with special characters on Unix shells
544+
var escapedArchivePath = archivePath.Replace ("'", "'\\''");
545+
var escapedDestinationPath = destinationPath.Replace ("'", "'\\''");
546+
514547
var psi = new ProcessStartInfo {
515548
FileName = "tar",
516-
Arguments = $"-xzf \"{archivePath}\" -C \"{destinationPath}\"",
549+
Arguments = $"-xzf '{escapedArchivePath}' -C '{escapedDestinationPath}'",
517550
UseShellExecute = false,
518551
CreateNoWindow = true,
519552
};

0 commit comments

Comments
 (0)