Skip to content

Conversation

@dignifiedquire
Copy link

No description provided.

@n0bot n0bot bot added this to iroh Nov 27, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Nov 27, 2025
@dignifiedquire dignifiedquire marked this pull request as ready for review November 27, 2025 12:15
@dignifiedquire dignifiedquire force-pushed the refactor-close-error-handling branch from a7363ea to ea37469 Compare November 28, 2025 09:51
Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't super like that the tests change... But the way they change still kind of make sense, so maybe it's fine?

Is the difference between actual ConnectionErrors vs. TransportError and LocallyClosed maybe that anytime you get a ConnectionError it's the "negotiated" error between you and the other side? Whereas you can return LocallyClosed immediately when you call Connection::close() from somewhere else, and similarly you can return TransportError immediately once you realize the certificate doesn't match.
But it doesn't necessarily mean that the connection error is the one you "negotiated" with the other side if that makes sense. (Where "negotiated" means it's the final error of the connection after you've drained it to make sure the other side hasn't reported a possibly different error?)

Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this! We should squash-merge this IMO. (I have the minor nit above, but that's easy enough to fix)

@dignifiedquire dignifiedquire force-pushed the refactor-close-error-handling branch from 1807225 to 9d1a2fa Compare November 28, 2025 12:42
@dignifiedquire dignifiedquire force-pushed the refactor-close-error-handling branch from 9d1a2fa to 9dcc6a0 Compare November 29, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🏗 In progress

Development

Successfully merging this pull request may close these issues.

4 participants