21 lines
2.8 KiB
Markdown
21 lines
2.8 KiB
Markdown
# Issues
|
||
|
||
1. (Major) File bytes are JSON-encoded — ~3–4× wire bloat + heavy CPU. StreamInstallFrame::FileChunk { bytes: Bytes } goes over the wire via serde_json (the Message impl). serde_json has no native byte type, so Bytes serializes as a JSON array of integers ([12,255,7,…]) — roughly 3–4 ASCII chars per payload byte, plus serialize/parse cost on both ends. The existing transfer path (peer.rs::stream_file_bytes) sends raw bytes straight on the QUIC stream for exactly this reason. For a feature whose entire purpose is moving file bytes efficiently on constrained peers, routing the payload through JSON undermines the goal. The 3 MiB S39 fixture hides it; a multi-GB game would not. Consider a binary codec (postcard/bincode) for the frame stream, or keep the control frames as-is but stream raw bytes for file content.
|
||
|
||
2. (Design) Solid archives → O(n²) extraction. Each file is pulled with a separate unrar p archive <file> (stream_unrar_file). For a solid .eti, unrar must re-decompress every preceding file on each invocation. solid is detected and sent in ArchiveBegin but never acted on. A solid multi-file archive could make streamed install pathologically slow. At minimum document the limitation; ideally a single-pass extraction for the solid case.
|
||
|
||
3. (Robustness) Single peer, no fallback. handle_stream_install_game_command picks one peer (peers.sort(); peers.into_iter().next()). If that peer declines (can_serve_game false) or the connection drops, the whole install fails — no retry against the other validated peers, unlike the normal multi-peer download. Fine for a prototype, worth a TODO.
|
||
|
||
4. (Minor) unrar p <name> is a wildcard mask. unrar treats the file argument as a pattern; a name containing */?/[ could match the wrong file or multiple files (multiple matches concatenate into stdout → corrupt stream). CRC32 would catch it as a failure rather than silent corruption, but it's a sharp edge. Worth noting / pinning exact-match if the unrar build supports it.
|
||
|
||
5. (Minor) CRC32 is optional. If unrar omits CRC32: (older builds, header-encrypted archives), crc32 is None and the receiver verifies size only.
|
||
|
||
6. (Minor / cosmetic)
|
||
- DownloadGameFileChunkFinished reports relative_path as "{game_id}/local/…" while bytes actually land in staging (.local.installing); and there's no total-size up front, so the UI can't show a percentage.
|
||
- parse_unrar_listing depends on unrar's human-readable lt output (Details:/Name:/Type:/Size:/CRC32:). -cfg- guards against user config, but it's still brittle across unrar versions/locales.
|
||
|
||
## Nits
|
||
|
||
- PeerStartOptions dropped #[derive(Debug)] for a hand-written Debug (Arc isn't Debug) — correct, and Clone/Default are retained. Good.
|
||
- commit() renames staging into local/, so the .lanspread_owned marker lands inside local/ — but that's identical to the existing install_inner behavior, so no regression.
|