Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Iroh Connection cannot adjust TransportConfig for outgoing connections #2872

Closed
CGamesPlay opened this issue Oct 31, 2024 · 1 comment · Fixed by #3111
Closed

Iroh Connection cannot adjust TransportConfig for outgoing connections #2872

CGamesPlay opened this issue Oct 31, 2024 · 1 comment · Fixed by #3111
Assignees
@flub
Labels
bug Something isn't working c-iroh

Comments

@CGamesPlay
Copy link

CGamesPlay commented Oct 31, 2024 •
edited
Loading

I was advised to create an issue from #2860 (reply in thread). Iroh-net's Endpoint::connect is hard-coded to use a TransportConfig of every-second keepalives with 30-second timeouts (latter default from Quinn), regardless of any TransportConfig that was passed to Builder::transport_config (which is only used for incoming connections). This effectively means that you can only increase the keepalive interval and reduce the timeout from these defaults.

I can't think of a great reason that the incoming and outgoing TransportConfig values should ever be different, so I think the default should be to use the Endpoint's configured values for outgoing connections. However, if there is a use case for this (perhaps different ALPNs should have different timeouts), a method like connect_with(node_addr, alpn, transport_config) could work well.
@flub flub added bug Something isn't working c-iroh labels Oct 31, 2024
@flub
Copy link
Contributor

flub commented Nov 5, 2024

My proposal would be to:

  • Add an Endpoint::connect_with_transport_config(&self, node_addr: impl Into<NodeAddr>, alpn: &[u8], transport_config: TransportConfig) (modulo some bikeshedding).
  • Modify connect_quinn to take the TransportConfig as well, so that the above can work. But in the first instance keep the current default TransportConfig but moved into connect, things work now for many folks and we need to be careful changing those values. I'd like to give folks a chance to try things out first before changing the defaults.
  • If Builder::transport_config is used use that as the default in connect as well as for accepting.

@ramfox ramfox moved this to 📋 Backlog in iroh Nov 27, 2024
flub added a commit that referenced this issue Jan 9, 2025
@flub
feat(iroh): Allow customising the TransportConfig for connections
0b31484 
Previously the TransportConfig was hardcoded for the
client (client/server in QUIC terminology: client initiates the
connection, server accepts the connection).  This rendered some
customisations on the server-side, as supported by the Builder,
impossible since the client would negotiate different limits.

Specifically the QUIC keep-alives were at a 1s or faster interval
while the connection timeout was at 30s or less.

This adds a new API marked as experimental which allows customising
the transport config for clients.

Secondly it also respects any custom TransportConfig set in the
Builder for clients as well as servers.  This is not a behaviour
change since the parameters set by default are now moved to the
Builder and due to how these are negotiated it does not matter that
the server will now also have the same values set by default.

Closes #2872.
@flub flub mentioned this issue Jan 9, 2025
Merged
4 tasks
@flub flub self-assigned this Jan 9, 2025
github-merge-queue bot pushed a commit that referenced this issue Jan 23, 2025
@flub
feat(iroh): Allow customising the TransportConfig for connections (#3111
2b92db4 
)

## Description

Previously the TransportConfig was hardcoded for the
client (client/server in QUIC terminology: client initiates the
connection, server accepts the connection).  This rendered some
customisations on the server-side, as supported by the Builder,
impossible since the client would negotiate different limits.

Specifically the QUIC keep-alives were at a 1s or faster interval
while the connection timeout was at 30s or less.

This adds a new API marked as experimental which allows customising
the transport config for clients.

Secondly it also respects any custom TransportConfig set in the
Builder for clients as well as servers.  This is not a behaviour
change since the parameters set by default are now moved to the
Builder and due to how these are negotiated it does not matter that
the server will now also have the same values set by default.

Closes #2872.

## Breaking Changes

### iroh

If Builder::transport_config is used it will now also affect initiated
connections, while before it only affected the accepted connections.

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [x] Self-review.
- [x] 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.
- [x] Tests if relevant.
- [x] All breaking changes documented.
@github-project-automation github-project-automation bot moved this to ✅ Done in iroh Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@flub flub

Labels
bug Something isn't working c-iroh
Projects
iroh
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(iroh): Allow customising the TransportConfig for connections n0-computer/iroh
2 participants
@flub @CGamesPlay