fix: bounded frame push prevents copy-mode scroll memory leak#190
Conversation
push_frame() used an unbounded mpsc::channel per client, creating a new String allocation per frame per push. During rapid scroll events in copy mode, each scroll set state_dirty, triggering a full frame rebuild (~500KB with cell content) pushed into the channel faster than the writer thread could flush to TCP. Measured: 8.5 MB → 1 GB in 1950 scroll events (22.8 MB/s, linear). Replace the unbounded channel-of-oneshots with a single-slot Arc<Mutex<Option<String>>> per client. push_frame() overwrites any unconsumed frame — only the latest frame matters for rendering. Memory is now bounded to O(clients) instead of O(frames). After fix: 8.5 MB → 16 MB flat across 2000 scroll events (0.2 MB/s). Includes test_scroll_memory.ps1 regression test that starts a detached session, injects 2000 scroll-up events via TCP, and asserts peak memory stays under 500 MB with growth rate under 10 MB/s.
|
Hey Aravind, really appreciate you taking the time to dig into this. Excellent work! I reproduced it myself before merging and the numbers matched almost exactly what you reported. On master, 1000 scroll events with backpressure took the server from 8.7 MB to 597 MB (about 602 KB per event). With your single slot fix in place, same scenario, it stayed flat at around 13 MB total. Night and day difference. The root cause analysis was spot on. The unbounded mpsc channel accumulated frame allocations faster than the writer thread could flush them to TCP, and with no backpressure mechanism the only limit was available RAM. Replacing it with an overwrite slot that keeps only the latest frame is exactly the right approach since intermediate frames are irrelevant for rendering anyway. Reviewed the full diff carefully:
Merged and built. Thanks for the clean PR and the detailed write up. This is a solid contribution. |
Summary
push_frame()used an unboundedmpsc::channelper client, allocating a newStringper frame per pushstate_dirty, triggering a full frame rebuild (~500KB with cell content) pushed faster than the writer thread can flush to TCPArc<Mutex<Option<String>>>per client —push_frame()overwrites any unconsumed frameChanges
src/types.rs: ReplaceFRAME_PUSH_SENDERS(unbounded channel vec) withFRAME_PUSH_SLOTS(single-slotFrameSlotvec). Dead slots pruned viaArc::strong_count.client_idtagging preserved for force-detach.src/server/connection.rs: Writer thread now polls frame slot alongside command response channel (5ms timeout). Command responses still delivered reliably viaresp_rx.tests/test_scroll_memory.ps1: Regression test — starts detached session, injects 2000 scroll-up events via TCP, asserts peak memory < 500 MB and growth rate < 10 MB/s.How to reproduce (before fix)
Test plan
cargo check— cleancargo test— all tests passpwsh tests/test_scroll_memory.ps1— 6 passed, 0 failed (peak 15.9 MB, 0.2 MB/s)