Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
| Report | Sat, August 31, 2024 at 20:43:12 UTC |
| Project | Firezone |
| Branch | fix/apple-path-monitoring |
| Testbed | github-actions |
Click to view all benchmark results
| Benchmark | Throughput | Throughput Results bits/s | (Δ%) | Throughput Lower Boundary bits/s | (%) |
|---|---|---|---|
| direct-tcp-client2server | ✅ (view plot) | 252,465,694.07 (+2.59%) | 237,759,103.16 (94.17%) |
| direct-tcp-server2client | ✅ (view plot) | 264,423,340.73 (+5.29%) | 243,268,389.51 (92.00%) |
| direct-udp-client2server | ✅ (view plot) | 303,285,184.35 (+4.62%) | 270,617,307.26 (89.23%) |
| direct-udp-server2client | ✅ (view plot) | 408,968,123.00 (+1.84%) | 388,020,662.46 (94.88%) |
| relayed-tcp-client2server | ✅ (view plot) | 258,394,837.66 (+3.63%) | 240,113,213.31 (92.92%) |
| relayed-tcp-server2client | ✅ (view plot) | 272,391,525.41 (+4.96%) | 249,517,901.50 (91.60%) |
| relayed-udp-client2server | ✅ (view plot) | 234,326,696.79 (+1.13%) | 220,603,657.82 (94.14%) |
| relayed-udp-server2client | ✅ (view plot) | 325,008,072.66 (-3.75%) | 317,181,570.74 (97.59%) |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
jamilbk
requested review from
ReactorScram,
conectado and
thomaseizinger
and removed request for
ReactorScram
jamilbk
force-pushed
the
fix/apple-path-monitoring
branch
from
8f4d7e5 to
74a147c
Compare
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
ACK modulo the unrelated(?) Elixir changes.
There was a problem hiding this comment.
I guess this shouldn't be part of the PR?
|
|
||
| // Hint to connlib we're back online, but only do so if our primary interface changes, | ||
| // and therefore we need to bump sockets. On darwin, this is needed to send packets | ||
| // Hint to connlib we're back online, but only do so if our connectivity has |
There was a problem hiding this comment.
The "hint" wording here is kind of outdated as "reset" is not conditional on anything, we just clear all network state.
| guard let path = path else { return true } | ||
| return path.supportsIPv4 != self.supportsIPv4 || | ||
| path.supportsIPv6 != self.supportsIPv6 || | ||
| path.gateways != self.gateways || |
There was a problem hiding this comment.
Are these always sorted?
There was a problem hiding this comment.
Fixed using Set since NWEndpoint implements Equatable.
| /// Used to avoid needlessly sending resets to connlib | ||
| private var primaryInterfaceName: String? | ||
| /// Used to avoid needlessly sending resets to connlib while still triggering reset | ||
| private var lastUpdatedPath: Network.NWPath? |
There was a problem hiding this comment.
| private var lastUpdatedPath: Network.NWPath? | |
| private var lastRelevantPath: Network.NWPath? |
|
|
||
| // We define a path as different from another if the following properties change | ||
| func connectivityDifferentFrom(path: Network.NWPath?) -> Bool { | ||
| guard let path = path else { return true } |
There was a problem hiding this comment.
So if the new path is null, we consider that different and do a reset? What is the point of that?
There was a problem hiding this comment.
Yeah, we need to initialize it somehow. There is a (highly unlikely) chance we can receive this callback, lastRelevantPath is nil, and we need to bump sockets so better to err on the side of one reset.
There was a problem hiding this comment.
You could initialise it (and call reset) without calling this function. That may be easier to understand how we handle that edge-case?
|
Your test failure is most likely fixed by #6497. |
jamilbk
force-pushed
the
fix/apple-path-monitoring
branch
from
666dafa to
fddd168
Compare
Co-authored-by: Thomas Eizinger <thomas@eizinger.io> Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
2 participants
On Apple, we try to be smart about triggering connlib's
reset()in order to keep from triggering endless update loops. This can happen because connlib itself triggers path monitoring updates through onUpdateRoutes and such.Before, we only kept track of whether our primary interface changed in order to consider the path updated. Now, we also track IPv4/IPv6 connectivity and the network's available gateways (read: routers) to trigger changes. This fixes the case where our interface loses or gains IPv4 / IPv6 connectivity, or the router address changes.
Fixes #6515