lib/config, lib/connections: Configurable protocol priority (ref #8626) #8868
Conversation
…thing#8626) This makes the various protocol priorities configurable among the other options. With this, it's possible to prefer QUIC over TCP for WAN connections, for example. Both sides need to be similarly configured for this to work properly. The default priority order remains the same as previously (TCP, QUIC, Relay, with LAN better than WAN). To make this happen I made each dialer & listener more priority aware, and moved the check for whether a connection is LAN or not into the dialer / listener -- this is the new "lanChecker" type that's passed around.
|
Somewhat orthogonal to your PR but IMHO mandatory if we want users to be able to properly replace TCP with QUIC: #7403 |
|
I'm not that happy with the priority approach to be honest. This puts the burden on the user to get the configuration right on all devices and prevents a hassle free upgrade path to prioritizing QUIC by default(if we want to do that). It would be better if the user could express that certain connection types have the same priority while being able to manipulate the order in which they are dialed. In essence we would sort by connection quality and for the same quality by dialing priority. With this approach QUIC WAN and TCP WAN would have the same connection quality but we'd still keep TCP with a higher dialing priority. This avoids connection flip-flops if one device is missing the configuration. We're also free to give QUIC a higher dialing priority by default in a future Syncthing release without causing havoc for existing clusters. |
|
I don't think that approach really fits, though. For one, we dial a whole bunch of addresses concurrently, IIRC. Also some method/addresses may require "warmup" (i.e., TCP punchthrough, in the rare cases when it works, requires connection attempts to have happened in both directions, I think). However I think we could accomplish what you want by adding a further config: an "upgrade threshold". Say, for example, that I configured priorities as:
and set the upgrade threshold to 10. Then we'd prioritise TCP when trying to get a connection, but if we already have a QUIC WAN connection and are considering a TCP WAN connection, then 35-30 = 5 which is less than the threshold of 10 so no upgrade is attempted, but we'd upgrade to any kind of LAN connection... Actually, that may be exactly the same as what you suggested, just expressed differently. |
|
The threshold approach sounds good 👍 |
There was a problem hiding this comment.
Looks good to me too. Just one thing that crossed my mind, definitely no blocker (plus the usual: likely missing some point, yada-yada...):
Do we really want to make LAN vs WAN prio configurable?
While senseless, one could configure LAN with worse prio than WAN as is. Maybe lets define that prio(type, lan) == prio(type, wan) - 10 instead. As far as I see that still fulfills all use-cases brought up, e.g. the threshold mechanism would still work fine with that. And it's a bit less config/moving pieces.
We do, or I do, because a use case for this is to use TCP locally but QUIC over WAN. That's not achievable with just an offset. |
|
Ah yes. there it is :) We might want to emit a warning on config validation when |
|
I added a thing to ensure that LAN is always preferred over WAN, for each individual protocol. It's still possible to say that we prefer TCP over WAN to QUIC over LAN, for example, and I think this is as it should be. |
* main: build(deps): bump github.com/quic-go/quic-go from 0.33.0 to 0.34.0 (syncthing#8877) gui, man, authors: Update docs, translations, and contributors lib/ignore: Properly handle non-existing included ignore-files (fixes syncthing#8764) (syncthing#8874) lib/connections: Avoid using nil lanChecker gui, man, authors: Update docs, translations, and contributors lib/config, lib/connections: Configurable protocol priority (ref syncthing#8626) (syncthing#8868) build: Upgrade recli (fixes syncthing#8503) (syncthing#8871)
* main: gui, man, authors: Update docs, translations, and contributors lib/ignore: Properly handle non-existing included ignore-files (fixes syncthing#8764) (syncthing#8874) lib/connections: Avoid using nil lanChecker gui, man, authors: Update docs, translations, and contributors lib/config, lib/connections: Configurable protocol priority (ref syncthing#8626) (syncthing#8868) build: Upgrade recli (fixes syncthing#8503) (syncthing#8871)
* main: (42 commits) all: Correct various typos (syncthing#8870) gui, man, authors: Update docs, translations, and contributors lib/model: Set platform data, incl. copying ownership, for new folders w/ NoPermissions flag (syncthing#8883) gui: Add Thai (th) translation template. (syncthing#8887) build: Produce nightly release builds gui, man, authors: Update docs, translations, and contributors build: Bump actions version; fix Node 12 deprecation warning (syncthing#8881) build: Build Debian packages build: Sign for upgrades build: Notarize mac builds build(deps): bump github.com/quic-go/quic-go from 0.33.0 to 0.34.0 (syncthing#8877) gui, man, authors: Update docs, translations, and contributors lib/ignore: Properly handle non-existing included ignore-files (fixes syncthing#8764) (syncthing#8874) lib/connections: Avoid using nil lanChecker gui, man, authors: Update docs, translations, and contributors lib/config, lib/connections: Configurable protocol priority (ref syncthing#8626) (syncthing#8868) build: Upgrade recli (fixes syncthing#8503) (syncthing#8871) build: Update dependencies (syncthing#8869) lib/model: Improve path generation for auto accepted folders (fixes syncthing#8859) (syncthing#8860) docs: fix typo (syncthing#8857) ...
st-review
added
the
frozen-due-to-age
This makes the various protocol priorities configurable among the other options. With this, it's possible to prefer QUIC over TCP for WAN connections, for example. Both sides need to be similarly configured for this to work properly.
The default priority order remains the same as previously (TCP, QUIC, Relay, with LAN better than WAN).
To make this happen I made each dialer & listener more priority aware, and moved the check for whether a connection is LAN or not into the dialer / listener -- this is the new "lanChecker" type that's passed around.