-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OpenConsole: Duplicate output lines when connecting to Ubuntu via SSH #15769
Comments
@j4james FYI I believe I found a bug in the LineFeed refactor. It's a tricky one and I'm not entirely sure yet how to debug this. I suspect I'll just have to do ye old "read the diff until you found the bug"-trick again. 😄 |
I found the exact line I need to flip to make it work again: fc95802#diff-21383c37a75bafb4ee95b4559a8f5b3d421640e19d3aef91cce9a419b28707e9L2103-R2178 In other words, if I revert the |
I don't get what's happening here. Who is making all those And this is assumedly not what the server is sending is it? Like can you do a My suggestion would be to add some logging to conhost that writes out everything that is received in the |
I spent quite some time trying to figure out what's going on and I'm not entirely sure which part is broken. FWIW I don't think it's the new |
What's also interesting is that I sometimes see a |
Hey, what version of openssh is that? They had an issue around the use of the alternate screen buffer where they would re-send an entire console write when the window size changed. |
Yeah, I was wondering that too. There are quite a few reports of duplicated content related to Win32-OpenSSH, but they all seem to point to a PR at PowerShell/openssh-portable#426 which was fixed years ago. |
I'm using The issue doesn't occur with WSL btw. As I've said before, I don't really think we're doing anything wrong per se, but it is weird that I cannot reproduce it before fc95802 and always reproduce it after fc95802 with Windows' SSH client. Something's whacky, I can feel it. 😄 |
Note that PR #14874 is definitely expected to change the behavior of the linefeed operation under certain conditions, but that would typically have involved margins in some way. My concern is that this particular change may not be expected, though, and there's no way of knowing that without seeing the API calls and/or escape sequences the ssh client is using. |
At this point I genuinely think it's a bug in Windows' SSH implementation. Here's a text file that 9 out of 10 times reproduces the issue in the latest OpenConsole version (1.18.1462.0) if I The shell doesn't play any role at all - it happens even if I use |
Y'ALL I FIGURED IT OUT!!!!!!!!!!!!!!!!!!!!! This works: Line 161 in c79298d
This does not: terminal/src/host/outputStream.cpp Line 101 in a1865b9
Replacing it with this fixes the issue: void ConhostInternalGetSet::SetViewportPosition(const til::point position)
{
auto& info = _io.GetActiveOutputBuffer();
const auto dimensions = info.GetViewport().Dimensions();
const auto windowRect = til::rect{ position, dimensions }.to_inclusive_rect();
info.SetViewport(Viewport::FromInclusive(windowRect), true);
} This happens because I would prefer writing THROW_IF_FAILED(info.SetViewportOrigin(true, position, true)); but it seems like Any thoughts about my findings? 😊 |
OK, that makes some sense. That sounds similar to the issue mentioned in PowerShell/openssh-portable#426. But note that
I'm inclined to agree with this, because aside from But note that Some things I'd want to look out for:
I think it should be fine, but I also thought |
(TIL: When you press Ctrl+Enter on GitHub it submits your comment. Whoops.)
This issue is basically another victim of #281. When we call This issue makes using SSH pretty much impossible to use with conhost and we intend to about +/- ship it next year, so we're on the hook to fix it within the next +/- couple months. 😟 I wonder if we should address #281 and remove the qirky behavior as well? It would break some new apps, but it would fix just as many old apps, including SSH. |
`SetConsoleWindowInfoImpl` calls `PostUpdateWindowSize`, which emits a `CM_SET_WINDOW_SIZE` event, which causes `_InternalSetWindowSize` to be called, which calls `ScreenBufferSizeChange` which then finally emits a `WINDOW_BUFFER_SIZE_EVENT` event into the client input buffer. This messes up applications like which make use of `WINDOW_BUFFER_SIZE_EVENT` to perform potentially lossy operations. In case of SSH this results in a resize (SIGWINCH) of the server-side screen which similarly may result in a response by the shell, etc. Since that happens over networks and is async, and because our conhost VT viewport implementation appears to have a number of subtle bugs, this results in duplicate output lines (sometimes hundreds). Under Windows Terminal this issue is not as apparent, since ConPTY has no viewport that can be moved and no scrollback. It only appears as an issue if a terminal application reacts poorly to the SIGWINCH event. Closes #15769 ## Validation Steps Performed * Set a breakpoint in `SynthesizeWindowBufferSizeEvent` * Launch WSL and cause the viewport to move down No calls to `SynthesizeWindowBufferSizeEvent` ✅ * Execute `tput reset` Input line moves to row 0 ✅
OpenConsole version
fc95802
Steps to reproduce
The exact steps to reproduce the issue are somewhat unclear to me, but it happens readily on a Ubuntu 22.04.2 LTS server I own. Access can be provided on a case-by-case basis. The issue occurs with both bash and fish-shell with the standard configuration provided by Ubuntu. It does not reproduce with Windows Terminal.
Expected Behavior
Actual Behavior
Like this:
Or any other wild intermix, including entirely missing or individually duplicated characters. Sometimes I receive up to 5000 lines of output this way.
Investigation
We do actually receive those duplicates as
WriteConsole
calls:git bisect
leads me to believe that this issue was introduced in fc95802. Unfortunately, the effect is entirely random and sometimes seems to not occur at all, so I can't be 100% certain. But I've tried to reproduce the issue with da3a33f ~100 times now and couldn't, whereas it happens at least half the time with fc95802.The text was updated successfully, but these errors were encountered: