Skip to content

fix: do a best-effort transmit on connection close#625

Open
colinmarc wants to merge 1 commit inton0-computer:mainfrom
colinmarc:main
Open

fix: do a best-effort transmit on connection close#625
colinmarc wants to merge 1 commit inton0-computer:mainfrom
colinmarc:main

Conversation

@colinmarc
Copy link
Copy Markdown

Description

If we're shutting down ungracefully, we can make a best-effort attempt to write the CONNECTION_CLOSE to the socket and notify the peer. If we're in the middle of graceful shutdown, it doesn't hurt, either.

This can be taken advantage of by callers to do a quicker shutdown if they know the drain period isn't necessary. Discussion here: n0-computer/iroh#4201

Copy link
Copy Markdown
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.

Thank you for looking into this @colinmarc :)

Before we put in more work, I need to check back with @flub whether our ideas in n0-computer/iroh#4201 are sound & desirable.

Other than that:

  • This needs a test! Ideally one that sets up a Runtime similar to the one we're using in iroh: Most importantly one that uses a TaskTracker and fairly aggressively closes all tasks (in this case those will be the connection and endpoint drivers).
  • Rather than driving the transmit in close, I'd like to drive the transmit only when the ConnectionDriver is dropped. Otherwise I worry the drive_transmit from the connection driver and the close call can get in each others way. I think there's an invariant that the connection driver is the only thing that calls drive_transmit and it never misses the consecutive calls to that function. The right place for calling drive_transmit might be in the impl Drop for ConnectionRef right after the conn.implicit_close(..).

@colinmarc
Copy link
Copy Markdown
Author

Before we put in more work, I need to check back with @flub whether our ideas in n0-computer/iroh#4201 are sound & desirable.

No worries, I just wanted to have something concrete to discuss :) This is only half of a workaround for that issue anyway.

The right place for calling drive_transmit might be in the impl Drop for ConnectionRef right after the conn.implicit_close(..).

That makes sense! Moved it to the drop for ConnectionRef. I wasn't sure if you meant inside the conditional, but that would mean we don't get this optimization for an explicit close/drop.

If we're shutting down ungracefully, we can make a best-effort attempt
to write the CONNECTION_CLOSE to the socket and notify the peer. If
we're in the middle of graceful shutdown, it doesn't hurt, either.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🚑 Needs Triage

Development

Successfully merging this pull request may close these issues.

2 participants