Hypercore p2p e2e test #1111
Hypercore p2p e2e test #1111
Conversation
| .wait_for_peers(expected, Duration::from_secs(30)) | ||
| .await?; | ||
| tracing::info!("Hypercore peers connected: {}", connected); | ||
| tokio::time::sleep(Duration::from_secs(3)).await; |
There was a problem hiding this comment.
This seems far less robust, might be the cause of the fails.
We should explicitly wait for peers to connect, not a 3 second sleep
|
|
||
| // Notify that first peer is connected (fire-and-forget if receiver dropped) | ||
| if let Some(tx) = peer_connected_tx.take() { | ||
| let _ = tx.send(()); |
There was a problem hiding this comment.
please log the error so we can surface it for debugging
ueco-jb
left a comment
ueco-jb
left a comment
There was a problem hiding this comment.
There are few different timeouts scattered through the code (30 and 60 seconds), I think they could be declared as const as well.
| /// Check if a port is available for binding | ||
| fn check_port_availability(port: u16) -> bool { | ||
| match std::net::TcpListener::bind(format!("127.0.0.1:{}", port)) { | ||
| Ok(_) => true, | ||
| Err(e) if e.kind() == io::ErrorKind::AddrInUse => { | ||
| tracing::warn!("Port {} is already in use", port); | ||
| false | ||
| } | ||
| Err(_) => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Same function in p2p.rs:669 binds to 0.0.0.0.
| // Decrement connection count when peer connection closes | ||
| connection_count_for_peer.fetch_sub(1, Ordering::SeqCst); |
There was a problem hiding this comment.
This variable is created locally but not used anywhere if I see correctly.
Similar situation with connection_count_for_swarm and connection_notify_for_swarm.
| tokio::time::sleep(poll_interval).await; | ||
|
|
||
| // Log progress every 5 seconds | ||
| if start.elapsed().as_secs().is_multiple_of(5) { |
There was a problem hiding this comment.
Interesting, that's something I didn't know before.
| if start.elapsed().as_secs().is_multiple_of(5) { | |
| if start.elapsed().as_secs() % 5 == 0 { |
Ah, I've read that the difference is is_multiple_of will not panic if you use 0 as modulus.
| match async_std::task::block_on(hyperswarm::run_bootstrap_node::<SocketAddr>(None)) { | ||
| let bind_addr = SocketAddr::new( | ||
| std::net::IpAddr::V4(std::net::Ipv4Addr::new(127, 0, 0, 1)), | ||
| 49737, |
There was a problem hiding this comment.
A magic number? Please assign a const.
| /// Check if a port is available for binding | ||
| fn check_port_available(port: u16) -> bool { | ||
| match std::net::TcpListener::bind(format!("0.0.0.0:{}", port)) { | ||
| Ok(_) => true, | ||
| Err(e) if e.kind() == io::ErrorKind::AddrInUse => { | ||
| tracing::warn!("P2P port {} is already in use", port); | ||
| false | ||
| } | ||
| Err(_) => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
The port is being binded, then the listener is dropped and the function returns only a bool. Before the actual bind happens, another process might snatch that port (because it's free in the mean time).
Effectively this check is kinda redundant?
Improves stability of the hypercore e2e test