diff --git a/README.md b/README.md index 69bc6ef..2ab9369 100644 --- a/README.md +++ b/README.md @@ -173,6 +173,16 @@ deliberately different: HTTP status, and no share link is printed. Go does not check the status: it takes a share ID from any body with exactly one space in it, so a 500 reply can still yield a share link. +- **A run whose servers are all down fails.** When no server picked with + `--server` answers its probe (the connection is refused, the network is + unreachable, the request times out or the backend answers wrongly), the run + ends with exit status 1 and `Terminated due to error:` naming each server + with its cause, and prints no report. The Go client prints an empty report + instead (`null` for `--json`, a `result` event carrying `null` for + `--json-stream`, an empty line for `--csv`) and exits 0; with `--json`, + `--json-stream`, `--csv` and `--simple` it prints nothing on stderr either, + so a run on a link that cannot reach the server looks like a success. When + some of the servers answer, both clients report those and exit 0. - **HTTP/1.1 by default, HTTP/2 behind `--http2`.** HTTP/2 carries every stream over one TCP connection, so `--concurrent` would stop meaning concurrent connections — and multiple connections is the standard way a speed test diff --git a/src/defs/server.rs b/src/defs/server.rs index 9021763..0160633 100644 --- a/src/defs/server.rs +++ b/src/defs/server.rs @@ -127,7 +127,7 @@ impl Server { Ok(u) => url_join_path(&u, &self.ping_url), Err(e) => { write_debug!(out, "Failed when creating HTTP request: {e}\n"); - return ServerStatus::default(); + return ServerStatus::down(format!("{e:#}")); } }; @@ -164,16 +164,27 @@ impl Server { "Failed when parsing get IP result: {}\n", GoQuote(&body) ); - return ServerStatus::default(); } + // Status first: a web server's 404 or 500 comes with an error + // page, and the status names that fault better than the body. + let down_reason = if status != StatusCode::OK { + Some(format!("the ping request returned HTTP {status}")) + } else if !body.is_empty() { + Some("the ping request returned a non-empty body".to_string()) + } else { + None + }; ServerStatus { - up: status == StatusCode::OK, - tls: facts.tls, + up: down_reason.is_none(), + down_reason, + // As before, a probe answered with a body reports nothing + // about its connection. + tls: facts.tls.filter(|_| body.is_empty()), } } Err(e) => { write_debug!(out, "Error checking for server status: {e:#}\n"); - ServerStatus::default() + ServerStatus::down(format!("{e:#}")) } } } @@ -622,9 +633,22 @@ const PROBE_BODY_PEEK: usize = 8 * 1024; #[derive(Debug, Default)] pub struct ServerStatus { pub up: bool, + /// Why the backend is down, when it is: the refused connection, the + /// unreachable network, the wrong status or a body where none belongs. + pub down_reason: Option, pub tls: Option, } +impl ServerStatus { + fn down(reason: String) -> Self { + ServerStatus { + up: false, + down_reason: Some(reason), + tls: None, + } + } +} + /// A running `--json-stream` progress ticker. struct ProgressTicker { stop: Arc, diff --git a/src/helper.rs b/src/helper.rs index c9c4a3a..fc8375e 100644 --- a/src/helper.rs +++ b/src/helper.rs @@ -63,6 +63,9 @@ pub async fn do_speed_test( let delimiter = cli.csv_delimiter_byte()?; let mut reps_json: Vec = Vec::new(); let mut reps_csv: Vec = Vec::new(); + let mut measured = false; + // Each server that did not answer its probe, and why. + let mut down: Vec<(String, String)> = Vec::new(); for current_server in servers { let tlog = TelemetryLog::new(); @@ -106,6 +109,16 @@ pub async fn do_speed_test( if servers.len() > 1 && !out.quiet { output::write_ui_blank(); } + down.push(( + format!( + "{} ({})", + output::sanitize(¤t_server.name), + output::sanitize(&hostname) + ), + status + .down_reason + .unwrap_or_else(|| "its probe failed".to_string()), + )); continue; } @@ -339,12 +352,20 @@ pub async fn do_speed_test( }); } + measured = true; + // Add a blank line after each test when testing multiple servers. if servers.len() > 1 && !out.quiet { output::write_ui_blank(); } } + // No server answered: fail the run. Go prints an empty report and exits 0, + // which a script takes for success: quiet mode hides the line saying why. + if !measured && !down.is_empty() { + return Err(none_responding(&down)); + } + if cli.csv { match report::csv_rows(&reps_csv, delimiter) { Ok(s) => write_out!("{s}"), @@ -377,11 +398,25 @@ fn report_failure(what: &str, e: anyhow::Error) -> anyhow::Error { e } +/// The error for a run whose servers were all down: each one and its cause. +fn none_responding(down: &[(String, String)]) -> anyhow::Error { + match down { + [(server, why)] => anyhow::anyhow!("Selected server {server} is not responding: {why}"), + _ => anyhow::anyhow!( + "None of the {} selected servers is responding: {}", + down.len(), + down.iter() + .map(|(server, why)| format!("{server}: {why}")) + .collect::>() + .join("; ") + ), + } +} + /// The reports as JSON sees them: `null` when nothing was measured. /// -/// Go declares a nil slice, which marshals to `null`, and both clients exit 0 -/// even when every server failed -- so the document is the only signal that -/// nothing happened, and a consumer testing for null must keep seeing it. +/// Go declares a nil slice, which marshals to `null`. Down servers fail the run +/// earlier, leaving a run with no server to test (`--server -1`, empty list). fn json_reports(reports: &[JSONReport]) -> Option<&[JSONReport]> { (!reports.is_empty()).then_some(reports) } diff --git a/tests/integration.rs b/tests/integration.rs index 65f4f5d..f5ee73f 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -149,6 +149,18 @@ fn handle(mut stream: TcpStream, telemetry_hits: Arc) -> std::io::R stream.write_all(body)?; stream.flush() }; + // What a real web server answers for a missing file or a broken script: + // the status, with an error page. + let error_page = |stream: &mut TcpStream, status: &str| -> std::io::Result<()> { + let page = format!("

{status}

\n"); + let head = format!( + "HTTP/1.1 {status}\r\nContent-Type: text/html\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + page.len() + ); + stream.write_all(head.as_bytes())?; + stream.write_all(page.as_bytes())?; + stream.flush() + }; match (method.as_str(), path.as_str()) { (_, "/empty.php") => respond(&mut stream, b"", "text/plain")?, @@ -176,6 +188,8 @@ fn handle(mut stream: TcpStream, telemetry_hits: Arc) -> std::io::R telemetry_hits.fetch_add(1, Ordering::Relaxed); respond(&mut stream, b"id 4815162342", "text/plain")? } + ("GET", "/error-page.php") => error_page(&mut stream, "404 Not Found")?, + ("GET", "/server-error.php") => error_page(&mut stream, "500 Internal Server Error")?, _ => { stream.write_all( b"HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\nConnection: close\r\n\r\n", @@ -1003,3 +1017,240 @@ fn a_failed_step_is_reported_with_its_cause() { "{stderr}" ); } + +/// A URL whose port nothing listens on, below the ephemeral range as with the +/// parity fixture's dead port, so no other test's socket can be given it. +fn dead_url() -> String { + let first = 20000 + (std::process::id() % 5000) as u16; + let port = (first..30000) + .find(|port| TcpListener::bind(("127.0.0.1", *port)).is_ok()) + .expect("a free port"); + format!("http://127.0.0.1:{port}/") +} + +/// Writes a server list and returns its path; each entry is `(id, name, +/// server URL, ping URL)`. +fn write_server_list(name: &str, servers: &[(i64, &str, String, &str)]) -> PathBuf { + let path = std::env::temp_dir().join(format!( + "librespeed-cli-test-{name}-{}.json", + std::process::id() + )); + let entries: Vec = servers + .iter() + .map(|(id, name, url, ping)| { + format!( + r#"{{"name":"{name}","server":"{url}","id":{id},"dlURL":"garbage.php","ulURL":"empty.php","pingURL":"{ping}","getIpURL":"getIP.php","sponsorName":"","sponsorURL":""}}"# + ) + }) + .collect(); + std::fs::write(&path, format!("[{}]", entries.join(","))).expect("write server list"); + path +} + +/// The error the run terminated with, from the line main prints for it; panics +/// when stderr has no such line. +fn failure(out: &Output) -> String { + const PREFIX: &str = "Terminated due to error: "; + let stderr = String::from_utf8_lossy(&out.stderr); + stderr + .lines() + .find_map(|line| line.strip_prefix(PREFIX)) + .unwrap_or_else(|| panic!("stderr has no {PREFIX:?} line:\n{stderr}")) + .to_string() +} + +// An unreachable server fails the run in every mode: exit 1, the server and the +// cause on stderr, no report on stdout. Go prints an empty report and exits 0. +#[test] +fn a_server_that_is_down_fails_the_run_in_every_mode() { + const DOWN: &str = "Selected server Dead (127.0.0.1) is not responding: "; + let list = write_server_list("down", &[(1, "Dead", dead_url(), "empty.php")]); + for mode in [ + &[][..], + &["--json"], + &["--json-stream"], + &["--csv"], + &["--simple"], + ] { + let mut args = vec!["--local-json", list.to_str().unwrap(), "--server", "1"]; + args.extend_from_slice(&["--duration", "1", "--no-icmp"]); + args.extend_from_slice(mode); + let out = run(&args); + + let stderr = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(1), "{mode:?}: {stderr}"); + assert_eq!(stdout_of(&out), "", "{mode:?}: no report"); + let msg = failure(&out); + // The cause is worded by reqwest and the OS; only its presence counts. + let cause = msg + .strip_prefix(DOWN) + .unwrap_or_else(|| panic!("{mode:?}: {msg}")); + assert!(!cause.is_empty(), "{mode:?}: the cause is named: {msg}"); + if mode.is_empty() { + assert!( + stderr.contains( + "\nSelected server Dead (127.0.0.1) is not responding at the moment, try again later\n" + ), + "{stderr}" + ); + } + } +} + +// A backend answering its probe wrongly is down, and the error says what it +// answered: a wrong status (even with an error page), otherwise the body. +#[test] +fn a_server_answering_its_probe_wrongly_fails_the_run_with_what_it_answered() { + let backend = MockBackend::start(); + for (ping, answered) in [ + ("missing.php", "HTTP 404 Not Found"), + ("error-page.php", "HTTP 404 Not Found"), + ("server-error.php", "HTTP 500 Internal Server Error"), + ("getIP.php", "a non-empty body"), + ] { + let list = write_server_list("probe-wrong", &[(1, "Wrong", backend.url(), ping)]); + let out = run(&[ + "--local-json", + list.to_str().unwrap(), + "--server", + "1", + "--no-icmp", + "--json", + ]); + assert_eq!(out.status.code(), Some(1), "{ping}"); + assert_eq!(stdout_of(&out), "", "{ping}"); + assert_eq!( + failure(&out), + format!( + "Selected server Wrong (127.0.0.1) is not responding: the ping request returned {answered}" + ), + "{ping}" + ); + } +} + +// With several servers all down, the error names each one with its own cause, +// not only the last one tried. +#[test] +fn a_run_whose_servers_are_all_down_names_each_with_its_cause() { + let backend = MockBackend::start(); + let list = write_server_list( + "all-down", + &[ + (1, "Dead", dead_url(), "empty.php"), + (2, "Wrong", backend.url(), "error-page.php"), + ], + ); + let out = run(&[ + "--local-json", + list.to_str().unwrap(), + "--server", + "1", + "--server", + "2", + "--duration", + "1", + "--no-icmp", + "--json", + ]); + let stderr = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(1), "{stderr}"); + assert_eq!(stdout_of(&out), "", "no report"); + let msg = failure(&out); + let causes = msg + .strip_prefix("None of the 2 selected servers is responding: ") + .unwrap_or_else(|| panic!("{msg}")); + // Split at the last "; ": the first cause's wording is not ours. + let (dead, wrong) = causes.rsplit_once("; ").unwrap_or_else(|| panic!("{msg}")); + let refused = dead + .strip_prefix("Dead (127.0.0.1): ") + .unwrap_or_else(|| panic!("{msg}")); + assert!(!refused.is_empty(), "the refusal is named: {msg}"); + assert_eq!( + wrong, + "Wrong (127.0.0.1): the ping request returned HTTP 404 Not Found" + ); +} + +// With more than one server, a run that measured any of them reports those and +// succeeds, as the Go client's does. +#[test] +fn a_down_server_among_others_leaves_the_run_to_the_ones_that_answer() { + let backend = MockBackend::start(); + let list = write_server_list( + "one-down", + &[ + (1, "Live", backend.url(), "empty.php"), + (2, "Dead", dead_url(), "empty.php"), + ], + ); + let out = run(&[ + "--local-json", + list.to_str().unwrap(), + "--server", + "1", + "--server", + "2", + "--duration", + "0", + "--no-download", + "--no-upload", + "--no-icmp", + "--json", + ]); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + let reports: serde_json::Value = serde_json::from_str(&stdout_of(&out)).expect("valid JSON"); + let names: Vec<&str> = reports + .as_array() + .expect("a report array") + .iter() + .map(|r| r["server"]["name"].as_str().unwrap()) + .collect(); + assert_eq!(names, ["Live"]); +} + +// Without --server the fastest server is picked and a down one skipped, as +// before: with every server down the run fails before any test. +#[test] +fn automatic_selection_with_every_server_down_fails_as_before() { + let backend = MockBackend::start(); + let list = write_server_list( + "auto-down", + &[ + (1, "Dead", dead_url(), "empty.php"), + (2, "Wrong", backend.url(), "error-page.php"), + ], + ); + let out = run(&[ + "--local-json", + list.to_str().unwrap(), + "--no-icmp", + "--json", + ]); + let stderr = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(1), "{stderr}"); + assert_eq!(stdout_of(&out), "", "no report"); + assert!( + stderr.contains("No server is currently available, please try again later.\n"), + "{stderr}" + ); +} + +// A server list that cannot be fetched fails as it always did. +#[test] +fn an_unreachable_server_list_still_fails_the_run() { + let url = format!("{}servers.json", dead_url()); + let out = run(&["--server-json", &url, "--server", "1", "--json"]); + let stderr = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(1), "{stderr}"); + assert_eq!(stdout_of(&out), ""); + assert!( + stderr.contains("Error when fetching server list: "), + "{stderr}" + ); + assert!(!failure(&out).is_empty(), "{stderr}"); +} diff --git a/tests/parity/cases.rs b/tests/parity/cases.rs index f2ee135..6a4d660 100644 --- a/tests/parity/cases.rs +++ b/tests/parity/cases.rs @@ -21,6 +21,7 @@ pub enum Why { CsvFormulas, CsvQuoting, TelemetryStatus, + DownServerFails, SchemelessPort, SchemeCase, NumbersRefused, @@ -47,6 +48,7 @@ impl Why { Why::CsvFormulas => "**CSV formulas are defused.**", Why::CsvQuoting => "**CSV quoting follows the csv crate.**", Why::TelemetryStatus => "**A telemetry reply other than 2xx fails the upload.**", + Why::DownServerFails => "**A run whose servers are all down fails.**", Why::SchemelessPort => "**Scheme-less server URLs with a port work.**", Why::SchemeCase => "**A server URL's scheme keeps its case.**", Why::NumbersRefused => "**Numbers the Go client cannot act on are refused.**", @@ -259,14 +261,31 @@ fn reports() -> Vec { /// A backend that is not there, and a telemetry server that is not either. fn failures() -> Vec { - let dead = |name, args: &[&'static str]| { - let mut all = vec!["--duration", "1", "--server", "2"]; + // Go exits 0, after an empty report where one was asked for; here the run + // fails and says why. + const DEAD: [&str; 4] = ["--duration", "1", "--server", "2"]; + // The cause is worded by reqwest and the OS, so it is left to `*`. + const DEAD_FAILS: &str = + "Terminated due to error: Selected server Fixture Dead (127.0.0.1) is not responding: *"; + let dead = |name, args: &[&'static str], go_report: Lines| { + let mut all = DEAD.to_vec(); all.extend_from_slice(args); listed(name, SERVERS, &all) + .stdout(&[Why::DownServerFails], go_report, &[]) + .stderr(&[Why::DownServerFails], &[], &[DEAD_FAILS]) + .exit(&[Why::DownServerFails], 0, 1) }; vec![ - dead("dead_json", &["--json"]), - dead("dead_csv", &["--csv"]), + listed("dead_plain", SERVERS, &DEAD) + .stderr(&[Why::DownServerFails], &[], &[DEAD_FAILS]) + .exit(&[Why::DownServerFails], 0, 1), + dead("dead_json", &["--json"], &["null"]), + dead( + "dead_json_stream", + &["--json-stream"], + &[r#"{"event":"result","reports":null}"#], + ), + dead("dead_csv", &["--csv"], &[""]), listed("server_not_in_list", SERVERS, &["--duration", "1", "--server", "99"]), listed("server_not_found_two", SERVERS, &["--server", "99", "--server", "98"]), // No --server: every server is probed, and one of them is dead. @@ -866,7 +885,15 @@ fn garbled() -> Vec { "--duration", "0", "--server", "2", "--no-download", "--no-upload", "--debug", "--simple", ], - ), + ) + .stderr( + &[Why::DownServerFails], + &[], + &[ + "Terminated due to error: Selected server Garbled probe (127.0.0.1) is not responding: the ping request returned a non-empty body", + ], + ) + .exit(&[Why::DownServerFails], 0, 1), telemetry("telemetry_500_id", "/telemetry-500-id.php") .stdout( &[Why::TelemetryStatus],