Skip to content

Commit

Permalink
fix(iroh-blobs): Remove debugging logs & more cleanup (#2690)
Browse files Browse the repository at this point in the history
## Description

More stuff to clean up I came across while trying to debug an issue.
Also, this makes it so that errors from `iroh_blobs::get::db::get_blob`
that happen *before* the `get_conn` step get printed.
<!-- A summary of what this pull request achieves and a rough list of
changes. -->

## Breaking Changes

None.
<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->
I decided to just remove the tracing logs in `downloader/progress.rs`.
They never really helped us surface the bug, they mostly polluted the
logs, to be honest.
The only thing they surfaced was that we didn't properly clean up
`TransferState::progress_id_to_blob` when there's an error in
`iroh_blobs::get::db::get_blob` that causes us to not send the
`DownloadProgress::Done` event.

## Change checklist

- [x] Self-review.
- ~~[ ] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.~~
- ~~[ ] Tests if relevant.~~
- [x] All breaking changes documented.
  • Loading branch information
matheus23 authored Sep 26, 2024
1 parent 19c8fd3 commit 857e513
Show file tree
Hide file tree
Showing 4 changed files with 5 additions and 7 deletions.
5 changes: 3 additions & 2 deletions iroh-blobs/src/downloader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,6 @@ impl<G: Getter<Connection = D::Connection>, D: Dialer> Service<G, D> {
entry.get_mut().intents.insert(intent_id, intent_handlers);
}
hash_map::Entry::Vacant(entry) => {
tracing::warn!("is new, queue");
let progress_sender = self.progress_tracker.track(
kind,
intent_handlers
Expand All @@ -728,7 +727,9 @@ impl<G: Getter<Connection = D::Connection>, D: Dialer> Service<G, D> {
);

let get_state = match self.getter.get(kind, progress_sender.clone()).await {
Err(_err) => {
Err(err) => {
// This prints a "FailureAction" which is somewhat weird, but that's all we get here.
tracing::error!(?err, "failed queuing new download");
self.finalize_download(
kind,
[(intent_id, intent_handlers)].into(),
Expand Down
3 changes: 0 additions & 3 deletions iroh-blobs/src/downloader/progress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ struct Inner {

impl Inner {
fn subscribe(&mut self, subscriber: ProgressSubscriber) -> DownloadProgress {
tracing::warn!(state=?self.state, "subscribe! emit initial");
let msg = DownloadProgress::InitialState(self.state.clone());
self.subscribers.push(subscriber);
msg
Expand Down Expand Up @@ -137,9 +136,7 @@ impl ProgressSender for BroadcastProgressSender {
// making sure that the lock is not held across an await point.
let futs = {
let mut inner = self.shared.lock();
tracing::trace!(?msg, state_pre = ?inner.state, "send to {}", inner.subscribers.len());
inner.on_progress(msg.clone());
tracing::trace!(state_post = ?inner.state, "send");
let futs = inner
.subscribers
.iter_mut()
Expand Down
2 changes: 1 addition & 1 deletion iroh-blobs/src/get/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,7 @@ pub enum DownloadProgress {
/// The offset of the progress, in bytes.
offset: u64,
},
/// We are done with `id`, and the hash is `hash`.
/// We are done with `id`.
Done {
/// The unique id of the entry.
id: u64,
Expand Down
2 changes: 1 addition & 1 deletion iroh-blobs/src/get/progress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ impl TransferState {
warn!(%id, "Received `Done` event for unknown progress id.")
}
}
_ => {}
DownloadProgress::AllDone(_) | DownloadProgress::Abort(_) => {}
}
}
}

0 comments on commit 857e513

Please sign in to comment.