Repository navigation
fix(net): avoid multiplayer content-transfer port conflicts - #3365
shamough1792 wants to merge 4 commits into
Conversation
Test builds for c63930e
|
isaacsa2
left a comment
There was a problem hiding this comment.
Reviewed 5e8edc7. Reserving UDP and TCP before announcing the host, retaining the listener through handoff and using exclusive Windows binds address the reported port conflict without changing the wire protocol or adding work to the driving loop. The bounded retry/error handling and reservation-lifetime tests cover the important failure paths. Current platform builds and workspace tests pass.
I am holding approval while Security audit is red. Its log reports the four existing zstd-family advisories, and #3367 now supplies the dependency-only fix with passing checks. Once that lands, please merge the updated main into this branch and rerun the locked checks and audit. Repeating the current run without the dependency update will not clear it.
The supplied two-computer test validates selection of port 27016 and loading a cached bus; it does not establish a complete cold transfer. I reviewed the code and CI, not a local two-computer session.
thibautgnt
left a comment
There was a problem hiding this comment.
Thanks @shamough1792, the reservation design is solid: binding TCP and UDP together before the session is announced and handing the very same TcpListener to the mods server removes the race cleanly, and the rollback and exhausted range tests are a nice touch.
- Moving off 27015 breaks LAN search and plain IP joins for these hosts.
LanSession::discoveronly broadcasts toDEFAULT_PORT(crates/omsi-net/src/lib.rs:1615, only the loopback targets coverPORT_RANGE), and a bare IP or host name join only tries 27015 (crates/omsi-net/src/join_target.rs:130 and :145). A host that now lands on 27016 (crates/omsi-net/src/host_ports.rs:103-150) stops answering "search the local network" and--lan-join 192.168.x.yfrom other computers. Before this PR, a Windows host with Apple Mobile Device Service still had UDP 27015, and remote players reached its wildcard TCP listener, so that combination worked on the LAN. That is a lot of Windows players (anyone with iTunes or Apple Devices installed). Suggested fix in this PR: make the join side try the whole range, which stays compatible with older hosts on 27015: sendDISCOVERto every port ofDEFAULT_PORT..DEFAULT_PORT + PORT_RANGEon the broadcast and subnet targets too (not only loopback), and give a portless IP or name join all ten addresses (join_addralready tries its addresses at once). Please also add to the description that an older client can only find such a host by code or byip:port. - Isaac's point: the audit job is red on this head because of the zstd advisories. Once #3367 is in main, please merge main into this branch and rerun the locked checks and audit.
Non-blocking:
- A dedicated server with a fixed
portnow refuses to start when only its TCP port is taken (it used to run with the mods transfer off). That is a reasonable choice, but the exit message in crates/omsi-app/src/lib.rs:507 still says "cannot open the session on UDP port", which will send server owners looking for the wrong thing. "UDP/TCP port" would do. - In
verify_loopback(crates/omsi-net/src/host_ports.rs:51-80), any error other than AddrInUse or PermissionDenied (for example a probe connect that is refused or a read that times out) ends hosting at once instead of trying the next port. Mapping probe failures to AddrInUse would keep the bounded retry.
Wire protocol and the ports opened stay the same as before (TCP was already listening on the session port whenever mods sharing was on), so nothing new is exposed to the internet.
Probe every automatic host port on broadcast and subnet targets and for portless IP/name joins, while preserving explicit destinations. Retry connection-level loopback probe failures without hiding resource exhaustion, and identify both transports in fixed-port startup errors.
|
Updated in
Validation: 174 network unit tests and 12 host-port integration tests pass; both lockfiles pass locked metadata checks and cargo-audit with zero vulnerabilities; size-budget and diff checks pass. The full locked workspace run completed with only the previously documented locale-dependent Pushed to the fork branch; CI should rerun on this head. |
What changes for the player?
Hosting reserves UDP state and TCP content transfers together, skipping an occupied candidate without stopping other applications. LAN search and portless IP/hostname joins now try all ten automatic host ports (27015-27024), so a host moved off 27015 remains reachable.
How to test
ip:port,hostname:port, bare numeric port, or session code still targets only the specified port. An older client must use a session code or explicitip:portto reach a host moved off 27015.OMSI_NO_LAN_MODSenabled, hosting remains UDP-only.Automated Windows x64 validation of review commit
c63930ec:cargo test -p omsi-net --locked -j 4 -- --test-threads=4: 174 unit tests and 12 host-port integration tests passed.cargo test --workspace --locked --no-fail-fast -j 4 -- --test-threads=4: completed; aggregate result lines report 3,555 passed, 1 failed and 156 ignored. The only failed test is the existingomsi-cfg::codepage::tests::detects_the_code_pagelocale-dependent assertion (details below). All networking tests passed.cargo build --release --locked -j 4: passed (7m 55s).cargo metadata --locked --format-version 1checks passed; neither lockfile contains an unused patch.bash scripts/size-budget.shpassed without increasing the baseline;git diff --checkpassed.Earlier manual evidence (before this review revision): the reporter completed a two-computer Windows test with TCP 27015 occupied, UDP/TCP 27016 selected, content serving started and the remote bus loaded, without Winsock 10055 /
ERR_NO_BUFFER_SPACE. The bus was cached (0 wanted), so this does not demonstrate a cold download or manual verification of this revision's new discovery paths.Screenshots
No graphical/layout change. Join-field and startup diagnostics now describe the actual port range/transports. Updated diagnostic screenshots have not been captured.
What does this change, and why?
TcpListenerto the mods service without closing/rebinding it. Windows sockets useSO_EXCLUSIVEADDRUSE; a protocol-free loopback probe verifies that the reserved listener is reachable.This is a generic networking boundary shared by all hosted sessions, not an Apple-specific or add-on-specific workaround. New add-ons require no special handling. No wire-protocol change or newly introduced listening port is involved; the mods service already used TCP on the session port.
Merged updated main at
b9935e4c(including the zstd security upgrade from #3367) through merge commit6ec29bd8; both lockfile audits are now clean. No advisory ignores are added.What could break?
Host startup requires TCP as well as UDP unless transfers are explicitly disabled. Fixed-port requests still fail instead of silently choosing another port. Firewall/forwarding rules must allow the actual selected port. Discovery sends a bounded ten-port fan-out per distinct target rather than only one broadcast port.
Older clients only search/join the default port automatically: for a host moved off 27015, use its session code or explicit
ip:port. Updated clients remain compatible with older hosts on 27015. Saves, settings, add-on formats and the wire protocol are unchanged.Windows x64 was validated locally; Linux/macOS/other architectures rely on CI. A fresh two-computer test of the new bare-IP/LAN-search behavior, cold full-bus transfer, and exhaustive add-on compatibility remain unverified. This does not claim to solve another application's system-wide socket/handle exhaustion. Missing content/fonts and synchronization-table mismatches are outside this port fix.
Known local baseline failure:
omsi-cfg::codepage::tests::detects_the_code_pagestill fails on this Traditional Chinese Windows machine (left: "咍曖",right: "ЛиАЗ"). The source is identical to upstream and not modified by this PR. All other executed tests passed; the full-workspace checkbox remains unchecked rather than hiding or patching this unrelated failure.Performance
No feature added. Bounded startup/discovery correctness fix; no frame-time or memory benchmark is claimed.
Machine, map and settings: Windows x64; the earlier manual two-computer test used Grundorf. No new driving benchmark was collected.
Checklist
cargo test --workspaceandcargo build --releasepassA new feature has its performance table filled in (this branch, the current release, 0.2.30)No feature added.crates/omsi-app/locales/app.yml, in every language I can (existing networking join explanations/startup diagnostics remain English; no new localized game UI)