Skip to content

Commit

Permalink
test(iroh-net): try fix flaky udp_blocked test
Browse files Browse the repository at this point in the history
This test is flaky because somehow the executor is entirely blocked
for a solid 5 seconds after which the entire netcheck report is timing
out.  Thus the part of the netcheck report that is being tested never
executes.

What blocks the executor however is a mystery to me.  This fix assumes
that it is generating the backtrace in the log, because there simply
isn't anything else that it can be.  Thus the relevant change is no
longer producing the backtrace.  The error message is already pretty
obvious.

To  go further, this disables the anyhow backtrace feature entirely.
This will probably make our lifes a bit harder in some situations but
there are many places where they end up in the log otherwise and each
time this happens it blocks an executor thread for a long time.

This also tweaks the error reporting to not generate so much logging
noise when the captive portal fails in an expected way.

## Notes & open questions

An attempt at fixing #1901
  • Loading branch information
flub committed Dec 20, 2023
1 parent 1389857 commit 0418af6
Show file tree
Hide file tree
Showing 9 changed files with 22 additions and 12 deletions.
3 changes: 0 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion iroh-base/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ rust-version = "1.72"
workspace = true

[dependencies]
anyhow = { version = "1", features = ["backtrace"] }
anyhow = { version = "1" }
bao-tree = { version = "0.9.1", features = ["tokio_fsm"], default-features = false, optional = true }
data-encoding = { version = "2.3.3", optional = true }
hex = "0.4.3"
Expand Down
2 changes: 1 addition & 1 deletion iroh-bytes/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ rust-version = "1.72"
workspace = true

[dependencies]
anyhow = { version = "1", features = ["backtrace"] }
anyhow = { version = "1" }
bao-tree = { version = "0.9.1", features = ["tokio_fsm"], default-features = false }
bytes = { version = "1.4", features = ["serde"] }
chrono = "0.4.31"
Expand Down
2 changes: 1 addition & 1 deletion iroh-gossip/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ workspace = true

[dependencies]
# proto dependencies (required)
anyhow = { version = "1", features = ["backtrace"] }
anyhow = { version = "1" }
blake3 = { package = "iroh-blake3", version = "1.4.3"}
bytes = { version = "1.4.0", features = ["serde"] }
data-encoding = "2.4.0"
Expand Down
2 changes: 1 addition & 1 deletion iroh-metrics/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ rust-version = "1.72"
workspace = true

[dependencies]
anyhow = { version = "1", features = ["backtrace"] }
anyhow = { version = "1" }
erased_set = "0.7"
http-body-util = "0.1.0"
hyper = { version = "1", features = ["server", "http1"] }
Expand Down
2 changes: 1 addition & 1 deletion iroh-net/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ workspace = true

[dependencies]
aead = { version = "0.5.2", features = ["bytes"] }
anyhow = { version = "1", features = ["backtrace"] }
anyhow = { version = "1" }
backoff = "0.4.0"
bytes = "1"
crypto_box = { version = "0.9.1", features = ["serde", "chacha20"] }
Expand Down
5 changes: 5 additions & 0 deletions iroh-net/src/netcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,11 @@ mod tests {
// create raw ICMP pings and we'll have to silenty accept this test is useless (if
// we could, this would be a skip instead).
let have_pinger = Pinger::new().is_ok();
if !have_pinger {
error!("pinger disabled, test effectively skipped");
} else {
info!("pinger enabled");
}

// This is the test: we will fall back to sending ICMP pings. These should
// succeed when we have a working pinger.
Expand Down
12 changes: 10 additions & 2 deletions iroh-net/src/netcheck/reportgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ impl Actor {
break;
}
tokio::select! {
biased;
_ = &mut total_timer => {
trace!("tick: total_timer expired");
bail!("report timed out");
Expand Down Expand Up @@ -533,6 +534,7 @@ impl Actor {
MaybeFuture {
inner: Some(Box::pin(async move {
tokio::time::sleep(CAPTIVE_PORTAL_DELAY).await;
debug!("Captive portal check started after {CAPTIVE_PORTAL_DELAY:?}");
let captive_portal_check = tokio::time::timeout(
CAPTIVE_PORTAL_TIMEOUT,
check_captive_portal(&dm, preferred_derp)
Expand All @@ -541,7 +543,14 @@ impl Actor {
match captive_portal_check.await {
Ok(Ok(found)) => Some(found),
Ok(Err(err)) => {
warn!("check_captive_portal error: {:?}", err);
let err: Result<reqwest::Error, _> = err.downcast();
match err {
Ok(req_err) if req_err.is_connect() => {
debug!("check_captive_portal failed: {req_err:#}");
}
Ok(req_err) => warn!("check_captive_portal error: {req_err:#}"),
Err(any_err) => warn!("check_captive_portal error: {any_err:#}"),
}
None
}
Err(_) => {
Expand Down Expand Up @@ -881,7 +890,6 @@ async fn run_probe(
/// return a "204 No Content" response and checking if that's what we get.
///
/// The boolean return is whether we think we have a captive portal.
#[allow(clippy::unnecessary_unwrap)] // NOTE: clippy's suggestion makes the code a lot more complex
async fn check_captive_portal(dm: &DerpMap, preferred_derp: Option<Url>) -> Result<bool> {
// If we have a preferred DERP node, try that; otherwise, pick a random one not marked as "Avoid".
let url = match preferred_derp {
Expand Down
4 changes: 2 additions & 2 deletions iroh/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ rust-version = "1.72"
workspace = true

[dependencies]
anyhow = { version = "1", features = ["backtrace"] }
anyhow = { version = "1" }
bao-tree = { version = "0.9.1", features = ["tokio_fsm"], default-features = false }
bytes = "1"
data-encoding = "2.4.0"
Expand Down Expand Up @@ -83,7 +83,7 @@ flat-db = ["iroh-bytes/flat-db"]
test = []

[dev-dependencies]
anyhow = { version = "1", features = ["backtrace"] }
anyhow = { version = "1" }
bytes = "1"
console-subscriber = "0.2"
duct = "0.13.6"
Expand Down

0 comments on commit 0418af6

Please sign in to comment.