Conversation
Even if there's an error we want to gracefully close the endpoint.
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/4007/docs/iroh/ Last updated: 2026-03-11T10:18:47Z |
| close_endpoint_with_timeout(&endpoint, output).await; | ||
| output.emit(PathStats::from_watcher(watcher)); |
There was a problem hiding this comment.
I think ideally you close the endpoint and only then emit path stats, otherwise you might be missing stats.
There was a problem hiding this comment.
ok, this makes me very sad. our stats APIs are still terrible. Did some kind of miserable fix. Probably.
There was a problem hiding this comment.
What?
It's a simple matter of fact that during Endpoint::close you generate traffic, thus path stats.
My proposal would be to just call Endpoint::close here. Calling Endpoint::close twice doesn't hurt anything.
There was a problem hiding this comment.
Calling Endpoint::close twice is indeed clever, but also an ugly hack. There really should just be a way to wait for the connection be drained. There is no more activity on the connection after that.
I was thinking of using Connection::closed first, but then realised it doesn't actually tell you the connection is drained. Because you close it locally it would just be completed right away.
Maybe the right solution is to have a Connection::drained method?
There was a problem hiding this comment.
Ok, so the reason I do things this way is because I want to emit the stats once the path is drained. And the only way to get that state is to go via the ConnectionInfo::closed thing. I think that is a shortcoming in our API design. But I'm not in a place to fix that here.
Is closing the endpoint twice better or worse than this hack? Both are hacks. To me this version expresses more the intent: emit the stats once the connection is drained.
wtf is this. out stats APIs are still miserable.
180903e to
b4cfedb
Compare
Description
Even if there's an error we want to gracefully close the endpoint.
I also added some extra logging.
Breaking Changes
n/a
Notes & open questions
Look, this is the code we're making everyone write now.
I appreciate this is just a quick fix, I did not check out the full structure of the example and try to organise it as nicely as possible. So this probably increases technical debt of this example a bit more.
Change checklist