attach: coalesce SIGWINCH bursts into one resize carrying the final size - #415
attach: coalesce SIGWINCH bursts into one resize carrying the final size#415dob323 wants to merge 1 commit into
Conversation
A fullscreen application under a dragging window delivers a stream of SIGWINCHes to the attach client; forwarding each one makes the attached application repaint per event, shoving a stale frame into the client's scrollback every time. Wait for the reported size to stop moving and for the signal queue to run dry, then forward a single resize with the final size. A lone resize still goes out after one settle interval. Regression test: attach through a real pty, count the WINCHes the session shell receives across a 24-step resize burst. On master several distinct resizes reach the session; patched, the burst lands as one.
|
I don't like the idea of swallowing SIGWINCH like this. If applications behave badly in the face of a SIGWINCH storm from the terminal emulator, that's an application problem. shpool should not be in the buisness of hiding this sort of thing. This could easily lead to subtlly different behavior for applications executing within shpool or outside of it. |
|
That is a fair principle, and I agree shpool should not paper over app bugs. Let me sharpen what we actually measured, because I think the harm is in shpool rather than in the application: during a resize burst, each forwarded event makes the daemon re-run its restore/repaint path, and each of those repaints lands a full stale frame in the client scrollback. Outside shpool the same drag just redraws in place — no scrollback pollution — so today it is shpool that behaves differently from a bare terminal, and the divergence users notice is the pile of dead frames after every window drag. Two narrower shapes, either of which drops the signal-swallowing you object to:
I would rework the PR to (1) unless you prefer (2) — or close it if you would rather not carry either. Which way do you want it? |
|
What do you mean by "the daemon re-run its restore/repaint path?" For ordinary SIGWINCH events, we should just be directly forwarding. We only play games with SIGWINCH on reattach. |
|
Do you mean that the repaint output ends up in the shpool_vt100 in-memory terminal that shpool maintains and then a bunch of repainted frames get dumped on reattach? If so, that sounds like it may be an issue with shpool_vt100 since we should be restoring the terminal to whatever state it would have been in response to the direct output of whatever application is issuing the repaints. I'm working on shpool-vterm, a replacement for shpool-vt100, but it's not ready for the prime time quite yet. |
|
Fair challenge — my wording was sloppy, so I went back to the code instead of arguing from memory. You are right that an ordinary resize is a direct forward: The asymmetry I was actually seeing is timing, not replay. During a drag, the client terminal is already at its final size (the kernel coalesces winsize on the client pty), but the burst replays serially through the socket, so the session pty lags behind reality — and the application repaints once per stale intermediate size into a terminal that has already moved on. Those wrong-size frames are what pile up in scrollback. Outside shpool the same app repaints track the current size at the moment each repaint happens, so intermediates look right when drawn and get overwritten in place. Given that, the honest description of this patch is: forward the size the terminal is actually at, once the burst settles, instead of replaying sizes that are already history. But I take the point that it changes signal delivery for apps that want every event. If a default-off config knob is acceptable I will rework it that way; if you would rather shpool not carry this at all, say so and I will close it without hard feelings — the fix matters most on our side for long restore buffers, and we can keep it as a local patch. |
|
Are you using AI to generate these responses? AI is only allowed for code in shpool, not for human communication, at least not without clearly sign-posting AI generated text. The signwinch chain as I understand it right now is: terminal emulator resizes -> sends SIGWINCH to the local shell process -> local shell forwards the signal to the forground process, which is ssh in this case -> ssh transmits the signal over its connection -> sshd picks up the transmitted signal and sends it to its process, which is either Are you saying that normally, this chain sends SIGWINCH to the shell running under sshd fast enough that there is some logic which debounces it, but shpool adds enough latency that the signals are no longer debounced? |
2 participants
A fullscreen application under a dragging window delivers a stream of SIGWINCHes to the attach client; forwarding each one makes the attached application repaint per event, shoving a stale frame into the client scrollback every time. This waits for the reported size to stop moving and for the signal queue to run dry, then forwards a single resize carrying the final size. A lone resize still goes out after one settle interval.
Regression test
regression::resize_burst_reaches_session_as_one_resize: attaches through a real pty and counts the WINCHes the session shell receives across a 24-step resize burst. On master several distinct resizes reach the session; patched, the burst lands as one.Running in production under our session manager for a week alongside #413 and #414. AI disclosure per the practice on #406: drafted with an AI assistant, reviewed and tested by a human before submission.