Some printed lines lost when resizing the terminal

joreiff commented 4 years ago

Describe the bug Resizing the terminal emulator may cause printed lines to be lost.

To Reproduce Change the height of the terminal while the following program is running:

import sys
import time

import enlighten

with enlighten.get_manager() as manager:
    with manager.counter(total=30) as counter:
        for i in range(30):
            time.sleep(1)
            print(i)
            counter.update()

Environment (please complete the following information):

Enlighten Version: 1.6.2
OS and version: Debian 10
Console application: GNOME Terminal 3.30.2 using VTE 0.54.2 +GNUTLS
Special Conditions: N/A

Additional context The same problem happens with tmux and other terminal emulators.

avylove commented 4 years ago

Thanks for reporting! I was able to reproduce the issue. It seems to be related to how the scroll area is set during a resize. Resizing was changed in 1.6.2 to deal with a bug in threaded programs. I'll need to do some debugging to figure out a fix.

avylove commented 3 years ago

The problem appears to be a result of #28. Resize handling was deferred to the next write action to avoid reentrant errors in threaded programs. This would be fine if the only thing that wrote was Enlighten, but that's not the case. So I'll need to revert that and maybe come up with another solution. Using 1.6.1 should be a good workaround until I can get a new release out.

joreiff commented 3 years ago

Thanks! I will downgrade to 1.6.1 then until 1.6.3 is released.

avylove commented 3 years ago

I put up some commits that should resolve the issue. Please test if you can. Still have a little more work to do before putting out a release.

joreiff commented 3 years ago

Thanks again! I just did some quick testing and it seems like the current master branch -- although not perfect -- is a definite improvement over both 1.6.1 and 1.6.2 in this regard. Whereas 1.6.2 (and also 1.6.1 as it just discovered) used to delete a line for every resize operation, now lines are only lost if you increase the height of the terminal. Shrinking the window, on the other hand, works correctly now.

avylove commented 3 years ago

Thanks for testing! I can't reproduce lost lines from growing the terminal. The only thing I found with both Gnome terminal and XTerm is, when the height is increased on a cleared terminal, there may be some blank lines inserted when increasing the terminal size. This doesn't occur once a page of the buffer has been filled and doesn't appear to result in any data loss.

joreiff commented 3 years ago

Interesting. I will try to debug this issue once I have some more time. I will let you know if I find something.

joreiff commented 3 years ago

Ok, I finally got around to do some more testing on the current master branch. The issue does not (or only very rarely) occur when I resize by dragging the terminal's lower edge with the mouse. It does, however, reliably occur when maximizing the terminal window (either via the window decorator button or using the keyboard shortcut). Can you reproduce the issue this way? I tested on both GNOME 3.30 and 3.38.

avylove commented 3 years ago

I tried for 20 minutes on GNOME Terminal 3.38.1 and I was only able to reproduce it once. I think it may be when the dimensions change in a certain ratio. Below is a modified version of your original code which prints the terminal dimensions. Can you try to reproduce like this and let me know the terminal size before and after the data loss?

import sys
import time

import enlighten

with enlighten.get_manager() as manager:
    with manager.counter(total=30) as counter:
        height_and_width = super(manager.term.__class__, manager.term)._height_and_width
        for i in range(30):
            time.sleep(1)
            print(i, height_and_width()[:2])
            counter.update()

joreiff commented 3 years ago

Now that is weird. Thanks to your code I have discovered that the terminal window shrinks every time I maximize and unmaximize it:

0 (49, 190)
1 (8, 96)
3 (49, 190)
4 (7, 95)
6 (49, 190)
7 (6, 94)
9 (49, 190)

I repeated the test with all GNOME Shell extensions disabled and obtained the same results, so this should not be a problem (GNOME 3.38, X11). As explained in a GNOME bug report, unmaximizing the terminal seems to be resizing the terminal twice because of some issue with GTK3. I can repeat the test tomorrow at work (GNOME 3.30, Wayland, different terminal emulators) to see if this may be the cause.

avylove commented 3 years ago

Interesting. I don't get that on 3.38 X11. I did discover that the issue seems to be timing, so I'll need to see if i can figure out a fix. If toggle() is placed before the sleep, everything is fine, which is probably why we don't see the issue every time. If the commented toggle() is used instead, no data is lost, but the buffer is missing between the scroll window and the progress bar. I'll see if I can figure out a solution.

import sys
import time

import enlighten

def toggle(num):
    if num % 2:    
        sys.stdout.write("\x1b[8;25;80t")
    else:
        sys.stdout.write("\x1b[8;26;80t")
    sys.stdout.flush()

with enlighten.get_manager() as manager:
    with manager.counter(total=30) as counter:
        height_and_width = super(manager.term.__class__, manager.term)._height_and_width
        for i in range(30):
            time.sleep(1)
            toggle(i)
            print(i, height_and_width()[:2])
            #toggle(i)
            counter.update()

avylove commented 3 years ago

This seems to be caused by the signal handler not completely executing before the next operation. Adding a delay of 0.01 - 0.02 seconds clears everything up. This is similar to the threading problem 1.6.2 sought to address. Re-adding that behavior should help in some cases, but not when writing is done outside of the library. There are essentially three ways to handle this:

1.) Wrap standard streams to check if resize is needed and call resize method if needed before writing

This can be done, but may be fragile for some special situations that don't use standard classes, though it could just be ignored for those and fall back to the old behavior. This would also solve the threading issue. Main downside is if someone is doing something non-standard with the standard streams, so we'd need to leave an option to avoid wrapping.

2.) Make a method in the manager to write through.

Some other libraries do this. It's easy to implement, but passes the work downstream since it requires modifying everything that writes to standard streams. I don't really like this option.

3.) Ignore these corner cases and consider resizing a best effort

This is reasonable, but I'm not really sure what downstream developers prefer

avylove commented 3 years ago

It looks like this may have been a red herring. The 0.02 second sleep after a resize is only needed when the resize is executed through terminal sequences and seems to be the processing time required for it to be detected and executed. Without the delay, the height is returned incorrectly from the terminal. This isn't an issue when the resize command is executed through subprocess.call().

I'll have another for any lingering issues. Were you able to verify the behavior on your work computer?

joreiff commented 3 years ago

Using the code from https://github.com/Rockhopper-Technologies/enlighten/issues/33#issuecomment-731768077: Note that I do not touch keyboard or mouse after starting the program... Maybe let's ignore that for now. :grin:

joreiff commented 3 years ago

And yes, the code from https://github.com/Rockhopper-Technologies/enlighten/issues/33#issuecomment-731746012 reproduces the results from https://github.com/Rockhopper-Technologies/enlighten/issues/33#issuecomment-731758684.

avylove commented 3 years ago

I reimplemented the threading code, but now it only defers resize if threading is explicitly true or multiple threads are detected. I still have something else I want to implement before a release, but let me know how the current code is working for you.

Here's an updated version of the test code that removes the testing issue.

import subprocess
import sys
import time

import enlighten

def toggle(num):
    if num % 2:
        subprocess.call(['/usr/bin/resize', '-s', str(25), '80'], stdout=subprocess.DEVNULL)
    else:
        subprocess.call(['/usr/bin/resize', '-s', str(26), '80'], stdout=subprocess.DEVNULL)

with enlighten.get_manager() as manager:
    with manager.counter(total=30) as counter:
        height_and_width = super(manager.term.__class__, manager.term)._height_and_width
        for i in range(30):
            time.sleep(1)
            toggle(i)
            print(i, height_and_width()[:2])
            #toggle(i)
            counter.update()

joreiff commented 3 years ago

I cannot test your updated example as /usr/bin/resize does not exist on my system. Manually resizing the terminal, however, works flawlessly with the current master branch. Thank you for all the effort!

avylove commented 3 years ago

resize is probably just in a different place on Ubuntu. No worries.

Just pushed out 1.7.0. Going to close this, but please report if you see any more issues. Thanks for all your help on this one!

joreiff commented 3 years ago

Thank you!

Rockhopper-Technologies / enlighten

Some printed lines lost when resizing the terminal #33