new: stream orig downloads, reap children on every path
The release-tarball download capped the whole request at 30 s (large tarballs on slow links always failed and fell through to worse origins) and buffered the entire body in memory: keep a 10 s connect timeout only and stream the body to the temp file. The bzip2 -dc child of a failing repack was neither killed nor waited on (zombie + open pipe); it is now reaped on both paths. git archive no longer pipes a stderr nobody drains (a chatty git deadlocked the archive) and any failure after the destination file was created removes the empty or partial tarball.
This commit is contained in:
+66
-21
@@ -12,7 +12,7 @@
|
|||||||
//! independently of the upstream sources.
|
//! independently of the upstream sources.
|
||||||
|
|
||||||
use std::ffi::OsString;
|
use std::ffi::OsString;
|
||||||
use std::io::{Read, Write};
|
use std::io::Read;
|
||||||
use std::path::{Path, PathBuf};
|
use std::path::{Path, PathBuf};
|
||||||
use std::process::{Command, Stdio};
|
use std::process::{Command, Stdio};
|
||||||
use std::time::Duration;
|
use std::time::Duration;
|
||||||
@@ -225,8 +225,16 @@ fn git_archive_tarball(
|
|||||||
.into());
|
.into());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Any failure after the destination file was created (spawn error,
|
||||||
|
// broken pipe, failed git) leaves behind an empty or half-written xz
|
||||||
|
// file that is not a valid tarball: it is removed before returning.
|
||||||
|
let outcome = (|| {
|
||||||
let file = std::fs::File::create(&dest)?;
|
let file = std::fs::File::create(&dest)?;
|
||||||
let mut encoder = XzEncoder::new(file, 6);
|
let mut encoder = XzEncoder::new(file, 6);
|
||||||
|
// git's stderr is inherited, not piped: nothing here drains a piped
|
||||||
|
// stderr, and a chatty git filling the 64 KiB pipe buffer would
|
||||||
|
// deadlock the archive — its diagnostics belong on the terminal
|
||||||
|
// anyway, like every other child process in this module.
|
||||||
let mut child = Command::new("git")
|
let mut child = Command::new("git")
|
||||||
.args([
|
.args([
|
||||||
"archive",
|
"archive",
|
||||||
@@ -236,21 +244,29 @@ fn git_archive_tarball(
|
|||||||
])
|
])
|
||||||
.current_dir(repo)
|
.current_dir(repo)
|
||||||
.stdout(Stdio::piped())
|
.stdout(Stdio::piped())
|
||||||
.stderr(Stdio::piped())
|
|
||||||
.spawn()
|
.spawn()
|
||||||
.map_err(|e| format!("failed to run 'git archive': {e}"))?;
|
.map_err(|e| format!("failed to run 'git archive': {e}"))?;
|
||||||
let mut stdout = child
|
let mut stdout = child
|
||||||
.stdout
|
.stdout
|
||||||
.take()
|
.take()
|
||||||
.ok_or_else(|| "git archive produced no output".to_string())?;
|
.ok_or_else(|| "git archive produced no output".to_string())?;
|
||||||
std::io::copy(&mut stdout, &mut encoder)
|
let copied = std::io::copy(&mut stdout, &mut encoder)
|
||||||
.map_err(|e| format!("cannot pipe git archive into '{}': {e}", dest.display()))?;
|
.map_err(|e| format!("cannot pipe git archive into '{}': {e}", dest.display()));
|
||||||
|
// The child is reaped whatever happened to the pipe, and a copy
|
||||||
|
// error only surfaces once it has been waited on. Closing our end
|
||||||
|
// first unblocks a git still writing into the pipe (SIGPIPE);
|
||||||
|
// otherwise `wait` could hang on it forever.
|
||||||
|
drop(stdout);
|
||||||
|
let status = child.wait()?;
|
||||||
|
copied?;
|
||||||
encoder
|
encoder
|
||||||
.finish()
|
.finish()
|
||||||
.map_err(|e| format!("failed to write '{}': {e}", dest.display()))?;
|
.map_err(|e| format!("failed to write '{}': {e}", dest.display()))?;
|
||||||
let status = child.wait()?;
|
Ok(status)
|
||||||
if !status.success() {
|
})();
|
||||||
// The half-written xz file is not a valid tarball: remove it.
|
match outcome {
|
||||||
|
Ok(status) if status.success() => {}
|
||||||
|
Ok(status) => {
|
||||||
let _ = std::fs::remove_file(&dest);
|
let _ = std::fs::remove_file(&dest);
|
||||||
return Err(format!(
|
return Err(format!(
|
||||||
"'git archive --format=tar {}' failed with status: {status} \
|
"'git archive --format=tar {}' failed with status: {status} \
|
||||||
@@ -259,6 +275,11 @@ fn git_archive_tarball(
|
|||||||
)
|
)
|
||||||
.into());
|
.into());
|
||||||
}
|
}
|
||||||
|
Err(error) => {
|
||||||
|
let _ = std::fs::remove_file(&dest);
|
||||||
|
return Err(error);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
log::info!(
|
log::info!(
|
||||||
"Created orig tarball from git archive of {tag}: {}",
|
"Created orig tarball from git archive of {tag}: {}",
|
||||||
@@ -340,28 +361,44 @@ fn fetch_and_repack(
|
|||||||
/// blocking client must not run on a tokio worker thread (scaffold is
|
/// blocking client must not run on a tokio worker thread (scaffold is
|
||||||
/// called from inside the async runtime), so the download runs on a plain
|
/// called from inside the async runtime), so the download runs on a plain
|
||||||
/// dedicated thread.
|
/// dedicated thread.
|
||||||
|
///
|
||||||
|
/// Only the connection carries a timeout (10 s, like the rest of the
|
||||||
|
/// codebase): a whole-request timeout would cap the ENTIRE download and
|
||||||
|
/// always fail large release tarballs on slow links. The body is instead
|
||||||
|
/// streamed to the temporary file chunk by chunk as it arrives, never
|
||||||
|
/// buffered whole in memory.
|
||||||
fn download_to_temp(url: &str) -> Result<PathBuf, Box<dyn std::error::Error>> {
|
fn download_to_temp(url: &str) -> Result<PathBuf, Box<dyn std::error::Error>> {
|
||||||
let url = url.to_string();
|
let url = url.to_string();
|
||||||
let contents = std::thread::spawn(move || {
|
let download = move || -> Result<PathBuf, Box<dyn std::error::Error + Send + Sync>> {
|
||||||
reqwest::blocking::Client::builder()
|
let mut response = reqwest::blocking::Client::builder()
|
||||||
.timeout(Duration::from_secs(30))
|
.connect_timeout(Duration::from_secs(10))
|
||||||
.build()?
|
.build()?
|
||||||
.get(&url)
|
.get(&url)
|
||||||
.send()?
|
.send()?
|
||||||
.error_for_status()?
|
.error_for_status()?;
|
||||||
.bytes()
|
|
||||||
})
|
|
||||||
.join()
|
|
||||||
.map_err(|_| -> Box<dyn std::error::Error> { "the download thread panicked".into() })??;
|
|
||||||
|
|
||||||
let temp = std::env::temp_dir().join(format!(
|
let temp = std::env::temp_dir().join(format!(
|
||||||
"pkh-orig-{}-{}",
|
"pkh-orig-{}-{}",
|
||||||
std::process::id(),
|
std::process::id(),
|
||||||
chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default()
|
chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default()
|
||||||
));
|
));
|
||||||
let mut file = std::fs::File::create(&temp)?;
|
let mut file = std::fs::File::create(&temp)?;
|
||||||
file.write_all(&contents)?;
|
if let Err(error) = response.copy_to(&mut file) {
|
||||||
|
// A failed download must not leave a partial temporary behind.
|
||||||
|
let _ = std::fs::remove_file(&temp);
|
||||||
|
return Err(error.into());
|
||||||
|
}
|
||||||
Ok(temp)
|
Ok(temp)
|
||||||
|
};
|
||||||
|
let downloaded: Result<PathBuf, Box<dyn std::error::Error + Send + Sync>> =
|
||||||
|
std::thread::spawn(download)
|
||||||
|
.join()
|
||||||
|
.map_err(|_| -> Box<dyn std::error::Error> { "the download thread panicked".into() })?;
|
||||||
|
// The thread must carry a `Send + Sync` error box; the auto traits are
|
||||||
|
// dropped on the way out.
|
||||||
|
match downloaded {
|
||||||
|
Ok(temp) => Ok(temp),
|
||||||
|
Err(error) => Err(error),
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Container/compression format of a tarball, detected from its content
|
/// Container/compression format of a tarball, detected from its content
|
||||||
@@ -508,12 +545,18 @@ fn repack_tarball_file(
|
|||||||
};
|
};
|
||||||
|
|
||||||
let result = repack_tar_stream(reader, name, upstream_version, &dest);
|
let result = repack_tar_stream(reader, name, upstream_version, &dest);
|
||||||
if let Some(mut child) = bzip2_child
|
if let Some(mut child) = bzip2_child {
|
||||||
&& let Ok(dest_path) = &result
|
if result.is_err() {
|
||||||
{
|
// The repack failed and nothing reads the child's stdout
|
||||||
|
// anymore: kill it (harmless if it already exited — bzip2 may
|
||||||
|
// be blocked writing into the unread pipe) and reap it,
|
||||||
|
// instead of leaking a zombie holding an open pipe.
|
||||||
|
let _ = child.kill();
|
||||||
|
let _ = child.wait();
|
||||||
|
} else {
|
||||||
let status = child.wait()?;
|
let status = child.wait()?;
|
||||||
if !status.success() {
|
if !status.success() {
|
||||||
let _ = std::fs::remove_file(dest_path);
|
let _ = std::fs::remove_file(&dest);
|
||||||
return Err(format!(
|
return Err(format!(
|
||||||
"'bzip2 -dc {}' failed with status: {status}",
|
"'bzip2 -dc {}' failed with status: {status}",
|
||||||
path.display()
|
path.display()
|
||||||
@@ -521,6 +564,7 @@ fn repack_tarball_file(
|
|||||||
.into());
|
.into());
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
}
|
||||||
if result.is_err() {
|
if result.is_err() {
|
||||||
// A failed repack must not leave a half-written tarball behind.
|
// A failed repack must not leave a half-written tarball behind.
|
||||||
let _ = std::fs::remove_file(&dest);
|
let _ = std::fs::remove_file(&dest);
|
||||||
@@ -703,6 +747,7 @@ fn repack_tar_stream(
|
|||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
use std::io::Write;
|
||||||
|
|
||||||
/// List the entry names of an xz tarball.
|
/// List the entry names of an xz tarball.
|
||||||
fn tarball_names(path: &Path) -> Vec<String> {
|
fn tarball_names(path: &Path) -> Vec<String> {
|
||||||
|
|||||||
Reference in New Issue
Block a user