Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Subshell output lost on window resize under tmux, GNU screen #3639

Closed
mc-butler opened this issue May 3, 2016 · 26 comments
Closed

Subshell output lost on window resize under tmux, GNU screen #3639

mc-butler opened this issue May 3, 2016 · 26 comments
Assignees
@aborodin
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress ver: 4.8.17 Reproducible in version 4.8.17
Milestone
4.8.18

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/3639
Reporter yurikhan (yuri.v.khan@….com)
Mentions and

To reproduce:

  1. Start xterm. Enable 256-color support: 
(xterm)$ set TERM=xterm-256color
  1. Start tmux with the following configuration enabling 256-color support: 
(xterm)$ cat tmux256.conf
set -g default-terminal "screen-256color"

(xterm)$ tmux -f tmux256.conf
  1. Start mc: 
(tmux)$ mc
  1. Hide panels by pressing Ctrl+O.
  1. Generate some subshell output: 
(mc)$ ls -al
  1. Resize the xterm window.

Expected behavior: shell output remains on screen (suitably padded or truncated to new window size).

Observed behavior: screen is cleared, with cursor remaining at the same position.

Versions:

  • Ubuntu 16.04
  • tmux 2.1
  • mc 4.8.15 compiled with libslang2 
$ mc -V
GNU Midnight Commander 4.8.15
Built with GLib 2.47.3
Using the S-Lang library with terminfo database
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm and Linux console
With support for X11 events
With internationalization support
With multiple codepages support
Virtual File Systems: cpiofs, tarfs, sfs, extfs, ext2undelfs, ftpfs, sftpfs, fish
Data types: char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

Note

Original attachments:
@mc-butler
Copy link
Author

Changed by yurikhan (yuri.v.khan@….com) on May 3, 2016 at 5:50 UTC (comment 1)

  • Version changed from master to 4.8.15

Cause:

  • Resizing the window causes a SIGWINCH sent to the mc process.
  • sigwinch_handler sets the flag mc_global.tty.winch_flag to 1.
  • The select call in feed_subshell is interrupted.
  • feed_subshell notices the winch_flag and calls tty_change_screen_size.
  • tty_change_screen_size calls SLsmg_reinit_smg.
  • SLsmg_reinit_smg sees that Smg_Mode is currently SMG_MODE_NONE (because it was set that way back in reset_smg that was called when panels were hidden).
  • SLsmg_reinit_smg calls SLsmg_init_smg.
  • SLsmg_init_smg calls init_smg_for_mode(SMG_MODE_FULLSCREEN).
  • init_smg_for_mode calls SLtt_init_video.
  • SLtt_init_video sends the smcup terminfo sequence to the terminal.
  • SLtt_init_video calls SLtt_init_keypad.
  • SLtt_init_keypad sends the smkx terminfo sequence to the terminal and flushes the output buffer. (If $TERM starts with xterm or exactly matches screen, this function returns immediately without flushing the buffer, so the issue will not be visible.)
  • The terminal switches to alternate screen and clears it.

@mc-butler
Copy link
Author

Changed by yurikhan (yuri.v.khan@….com) on May 3, 2016 at 5:54 UTC

@mc-butler
Copy link
Author

Changed by yurikhan (yuri.v.khan@….com) on May 3, 2016 at 5:56 UTC (comment 2)

The patch above adds a check for how == QUIETLY to skip the call to tty_change_screen_size when panels are hidden.

This fixes the problem for me and seems not to introduce any regression with the ncurses build. I have not tested it extensively, though.

@mc-butler
Copy link
Author

Changed by yurikhan (yuri.v.khan@….com) on May 3, 2016 at 6:02 UTC (comment 3)

I should probably add that libslang2 is version 2.3.0 here.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on May 4, 2016 at 11:33 UTC (comment 4)

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.17

Branch: 3639_subshell_output_lost
[d4e95d36aa39db5ca0ce34d47a8d2b94b1771cbe]

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on May 4, 2016 at 18:51 UTC (comment 5)

I've pushed amended commit as [a435037bad30c7e5dafbe31c0f3cc38230c459a9], because your commit message formatting is unreadable, do you think you could take that instead?

Otherwise, I can surely vote for the branch, because it looks innocent enough, but generally this kind of patches always makes me feel bad: sadly, we don't have any integration tests to cover this, and not only it's unclear if this will subtly break something or not, but also it's not guaranteed that it will not be broken by some other change in the future :-/

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on May 4, 2016 at 18:53 UTC (comment 6)

  • Votes set to zaytsev

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on May 4, 2016 at 19:09 UTC (comment 7)

  • Branch state changed from on review to approved

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on May 5, 2016 at 7:00 UTC (comment 8)

  • Status changed from accepted to testing
  • Resolution set to fixed
  • Votes changed from zaytsev to committed-master
  • Branch state changed from approved to merged

Merged to master: [c4a8882].

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on May 5, 2016 at 7:02 UTC (comment 9)

  • Status changed from testing to closed

@mc-butler
Copy link
Author

Changed by yurikhan (yuri.v.khan@….com) on May 5, 2016 at 10:27 UTC (comment 5.10)

Replying to zaytsev:

this kind of patches always makes me feel bad: sadly, we don't have any integration tests to cover this, and not only it's unclear if this will subtly break something or not, but also it's not guaranteed that it will not be broken by some other change in the future :-/

Apparently it was broken ever since the migration to S-Lang, and nobody noticed or got sufficiently annoyed. I did only because I use a tiling window manager which tends to slightly resize windows every time a new window appears.

@mc-butler
Copy link
Author

Changed by zaytsev-work (@zyv) on May 9, 2016 at 11:24 UTC (comment 11)

  • Version changed from 4.8.15 to 4.8.17
  • Votes committed-master deleted
  • Milestone changed from 4.8.17 to 4.8.18
  • Branch state changed from merged to no branch
  • Resolution fixed deleted
  • Status changed from closed to reopened

Ha-ha-ha, I did have a bad feeling about this patch, and, sure enough, right after release, I have bisected a very annoying bug as being caused by [c4a888], f%&@*!

So, I have i3 / gnome-terminal on Ubuntu 14.04 here:

1) Switch to an empty workspace
2) Create a new terminal (Super + Enter)
3) Start mc
4) Switch to subshell with Ctrl + O
5) Create a new horizontal split with Super + Enter
6) Start vim in mc subshell

Press Ins repeatedly to enjoy watching vim absolutely confused about the window size and completely unusable. What's more annoying is that an explicit WINCH via Super + F doesn't help it.

@yurikhan, you're saying that you're using a tiling window manager, can you reproduce it?

Not an absolutely critical show-stopper, but unless there is a better solution, I would rather revert it altogether... mc is still losing subshell output in screen / tmux on Ctrl + O, so screen / tmux users are accustomed to constant pain anyways.

@mc-butler
Copy link
Author

Changed by yurikhan (yuri.v.khan@….com) on May 9, 2016 at 11:42 UTC (comment 11.12)

Replying to zaytsev-work:

enjoy watching vim absolutely confused about the window size and completely unusable.

Let’s minimize your recipe:

  1. Start a terminal emulator.
  2. Start mc in it.
  3. Switch to subshell (Ctrl+O).
  4. echo $COLUMNS $LINES
  5. Resize the window.
  6. echo $COLUMNS $LINES

Expected: $COLUMNS and $LINES track the current window size.

Observed: $COLUMNS and $LINES stay the same.

What's more annoying is that an explicit WINCH via Super + F doesn't help it.

Presumably because the WINCH goes to MC which, because of this patch, does not pass it downstream.

Not an absolutely critical show-stopper, but unless there is a better solution, I would rather revert it altogether...

I will work on a better solution.

mc is still losing subshell output in screen / tmux on Ctrl + O, so screen / tmux users are accustomed to constant pain anyways.

Still losing, with the previous patch? Tell me more. I was intending to end this pain with this and #3640.

@mc-butler
Copy link
Author

Changed by yurikhan (yuri.v.khan@….com) on May 9, 2016 at 12:08 UTC (comment 13)

Please revert [c4a888] and try this one instead.

@mc-butler
Copy link
Author

Changed by yurikhan (yuri.v.khan@….com) on May 9, 2016 at 12:14 UTC

A less invasive attempt

@mc-butler
Copy link
Author

Changed by zaytsev-work (@zyv) on May 10, 2016 at 8:30 UTC (comment 14)

Hi Yuri, thanks for getting back to me!

Please revert [c4a888] and try this one instead.

I have reverted the old patch and tried the new one, and it seems to fix the problem that I'm observing.

As mentioned above, do you have any idea on how this can be tested in an automated fashion, so that it doesn't get broken next time any "fixing" is done on the SLang layer? One could possibly try to set up integration tests with mc2, but it's not merged yet and frankly speaking I have no idea as to if and when this will happen. On the unit test level, one could possibly try to check mock behaviors if the code was encapsulated well enough, but I'm not sure it's even practical at the moment.

Still losing, with the previous patch? Tell me more. I was intending to end this pain with this and #3640.

I'm not exactly sure what the intent of #3640 was, but I understood that it had to do something with cursor position on window resizes, which, apparently, is important for some reason.

In my case, I wasn't losing subshell output upon window resizes, but upon panel flips in screen, even if the window size stays constant. I can reproduce it with 4.8.17 on Ubuntu 14.04 & screen as follows:

  1. Start screen
  2. Start mc
  3. Ctrl+O to hide the panels
  4. Run something like ls -als
  5. Ctrl+O to show panels
  6. Ctrl+O to hide panels
  7. Observe that subshell output is gone

This has been around for a very long while, and it seems that there is no change with #3640.

@mc-butler
Copy link
Author

Changed by zaytsev-work (@zyv) on May 10, 2016 at 8:39 UTC (comment 15)

I have to add that I've just tried tmux, and, to my amazement, I can confirm that it doesn't loose subshell output, be it 4.8.16 or 4.8.17... not sure about cursor position, I haven't got time to check this any more attentively yet.

Maybe I should bit the bullet and ditch screen for tmux, even though last time I tried it, I failed, because tmux felt painfully slow for some reason, and I'm just too used to screen quirks.

@mc-butler
Copy link
Author

Changed by yurikhan (yuri.v.khan@….com) on May 10, 2016 at 9:54 UTC (comment 16)

Replying to zaytsev-work:

As mentioned above, do you have any idea on how this can be tested in an automated fashion, so that it doesn't get broken next time any "fixing" is done on the SLang layer?

Perhaps mock the terminal and possibly the user, and add lots of test cases like:

  • Given a terminal in initial state,

when tty_change_screen_size() is called,

then the terminal is still in main screen;

  • Given a terminal in alternate screen,

when tty_change_screen_size() is called,

then the terminal is still in alternate screen;

  • Given a terminal in alternate screen,

when panels are redrawn,

then the terminal has never received a “save cursor position” sequence;

etc.

Unfortunately I cannot propose a fully ready-made solution for mocking the terminal. I know of this Python library called pyte which might be up to the task, but it would need to be somehow integrated with the test framework. In fact, even a C terminal emulation library would need non-trivial integration.

I'm not exactly sure what the intent of #3640 was, but I understood that it had to do something with cursor position on window resizes, which, apparently, is important for some reason.

It is important because losing the cursor position causes the next page of output to overwrite the last one.

In my case, I wasn't losing subshell output upon window resizes, but upon panel flips in screen, even if the window size stays constant. […] This has been around for a very long while, and it seems that there is no change with #3640.

Is your screen configured for alternate screen switching? By default, it is not; you’d need an altscreen on line in your .screenrc. tmux, on the other hand, does alternate screen out of the box.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on May 12, 2016 at 16:14 UTC (comment 17)

Is your screen configured for alternate screen switching? By default, it is not; you’d need an altscreen on line in your .screenrc. tmux, on the other hand, does alternate screen out of the box.

Hi Yuri, thanks again for your input!

I've tried it with altscreen on, and screen doesn't loose the output with this patch anymore, also when the window is resized :-) Moreover, out of curiosity, I've also tried 4.8.16, and managed to trigger a segfault after a few resizes o_O You've done a great job!

Unfortunately I cannot propose a fully ready-made solution for mocking the terminal. I know of this Python library called pyte which might be up to the task, but it would need to be somehow integrated with the test framework.

Oh, I see, that's a nice idea. I've also heard of pyte, but I didn't realize that it can be very useful for this purpose.

The integration in the testing framework is the easiest part. Autotools come with a simple test harness out of the box, basically, all you need is a script that returns zero on success, and non-zero on failure. I can certainly hack something up and get Travis to run it.

The most difficult part for me would be to write the test cases themselves :-/ I'm afraid that unless you can lend us a hand, in the end, we'll just have to commit this one as usual, without tests... Oh well.

@mc-butler
Copy link
Author

Changed by yurikhan (yuri.v.khan@….com) on May 13, 2016 at 11:07 UTC (comment 17.18)

Replying to zaytsev:

Autotools come with a simple test harness out of the box, basically, all you need is a script that returns zero on success, and non-zero on failure.

That looks not very hard but annoyingly fiddly.

What we have is a system consisting of two components, the terminal emulator and the application, connected by an RPC channel with an escape sequence–based protocol over stdin/stdout. Moreover, there are two implementations of the protocol (ncurses and slang) and they do not generate strictly equivalent traffic.

A test case would contain some setup/teardown code on both sides of the channel. Additionally, it would need to perform actions on the application side and observe effects on the terminal side, or vice-versa. Assuming pyte as the virtual terminal emulation library, we need the following sequence:

  1. The testing script creates a virtual terminal and performs terminal-side test case setup.
  2. The testing script forks off a subprocess with its stdin/stdout connected to the virtual terminal and TERM set to something appropriate.
  3. The subprocess performs application-side test case setup.
  4. The subprocess performs the main test case action (e.g. tty_change_screen_size()).
  5. ???
  6. The testing script checks a terminal-side effect (e.g. an smcup sequence has never been received).
  7. ???
  8. The subprocess performs application-side teardown and exits.
  9. The testing script performs terminal-side teardown.
  10. The testing script collects the test result.

The ??? in steps 5 and 7 indicate some kind of interprocess synchronization, so that the outer script knows when to check the result and then the subprocess knows when to resume teardown.

Maybe we could hijack stdin/stdout in-process and just analyze the bytes that are written on stdout. Maybe there is a virtual terminal library that is usable from C, also in-process. This would simplify testing because that kind of synchronization wouldn’t be necessary.

@mc-butler
Copy link
Author

Changed by yurikhan (yuri.v.khan@….com) on May 22, 2016 at 6:28 UTC (comment 19)

I might have made an impression that I’m working on automated tests for this ticket. I’m sorry but I’m not. IMO that should be a different ticket.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jun 13, 2016 at 6:25 UTC (comment 20)

  • Branch state changed from no branch to on review

Branch: 3639_subshell_output_lost_fix
Initial [e41401dde118a005dcd627bc286e3e747c0b80c1]

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jun 28, 2016 at 6:37 UTC (comment 21)

  • Branch state changed from on review to approved
  • Votes set to andrew_b

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jun 28, 2016 at 6:38 UTC (comment 22)

  • Branch state changed from approved to merged
  • Votes changed from andrew_b to committed-master
  • Status changed from reopened to closed
  • Resolution set to fixed

Merged to master: [57d7130].

git log --pretty=oneline cb06f8c..57d7130

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 10, 2016 at 17:28 UTC

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jul 10, 2016 at 17:30 UTC (comment 23)

  • Cc set to and

Unit test by Michael Gold <​michael@bitplane.org>, I wish we could add this to the check thing...

https://bugs.debian.org/825974

Still too overwhelmed to try to address it myself :-(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aborodin aborodin

Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress ver: 4.8.17 Reproducible in version 4.8.17
Milestone
4.8.18
Development

No branches or pull requests
2 participants
@aborodin @mc-butler