fix: prevent hung archiver fetches from stopping nodelist refresh - #28
fix: prevent hung archiver fetches from stopping nodelist refresh#28jairajdev wants to merge 1 commit into
Conversation
A hung archiver HTTP request could block the nodelist refresh loop indefinitely, leaving the proxy serving a stale node list for days. - Add reqwest client timeouts to nodelist refresh and archiver discovery using max_http_timeout_ms - Log fetch, read, and parse failures to stderr so PM2 error logs capture archiver issues - Run nodelist refresh and archiver discovery in spawned tasks so the interval ticker is not blocked - Continue to the next archiver when a fetch fails or times out instead of stalling the loop
| ticker.tick().await; | ||
| _liberdus.update_active_nodelist().await; | ||
| let liberdus = Arc::clone(&_liberdus); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Spawning unnecessary task, as this scope is already inside a tokio multi threaded task, This will resut in O(n) task grow from original O(1) task each iteration of loop.
Just do this.
| tokio::spawn(async move { | |
| _liberdus.update_active_nodelist().await; |
Main file doesn't need to be touch at all for this PR
There was a problem hiding this comment.
Please keep the main file untouched.
| ticker.tick().await; | ||
| Arc::clone(&_archivers).discover().await; | ||
| let archivers = Arc::clone(&_archivers); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Same review as spawning nodelist update
|
|
||
| tokio::spawn(async move { | ||
| let resp = match reqwest::get(url).await { | ||
| let client = match reqwest::Client::builder() |
There was a problem hiding this comment.
Building a http client for every task spawn in for loop is extremely expensive and cost thread processing time.
Move this line out of the for loop and Atomic reference clone it if you're gonna go builder pattern.
Otherwise just use request::get(url)
| } | ||
| }; | ||
| let body: Result<SignedArchiverListResponse, _> = | ||
| serde_json::from_str(&resp.text().await.unwrap()); |
There was a problem hiding this comment.
I genuinely think using unwrap(); in my previous implementation was the actual problem and none of the new code is necessary if we could just gracefully handle the unwrap();
What I think happened is unwrap() introduce a panic that poison the background recurring tokio task loop and the loop break off.
@jairajdev So all you have to do is use a match arm in unpacking that resp.text() instead of unwrap(). That would just solve the entire problem without all this code changes. Because one iterative failure only stale for about 60sec and the next iteration of discovery will kick in due to infinite loop in the main. The reason why I think unwrap(); is the culprit is that it break the loop and diminishes entire background thread.
2 participants
A hung archiver HTTP request could block the nodelist refresh loop indefinitely, leaving the proxy serving a stale node list for days.