Fix crash on terminal resize during download

[dmach]

Resolves #972

[marcus-h]

On 2021-12-27 06:22:09 -0800, Daniel Mach wrote: diff --git a/osc/meter.py b/osc/meter.py index 3372a1a1..fa7a99a0 100644 --- a/osc/meter.py +++ b/osc/meter.py @@ -10,19 +16,49 @@ have_pb_module = False +if have_pb_module: + class ProgressBar(pb.ProgressBar): + """ + Modified ProgressBar that sets term_width argument to avoid setting SIGWINCH signal handler. + + Background: + We're doing this because the download routines are unable to restart interrupted syscalls. + See signal(7) for more details: + Interruption of system calls and library functions by signal handlers

Hmm it is rather due to a "bug" in m2crypto: m2.ssl_read returns None if the SSL connection has a specific state and a read(...) syscall was interrupted, which is indirectly issued by SSL_read(...). It is just that M2Crypto.SSL.Connection.recv_into does not handle None. (In our specific blocking IO case, the desired behavior would probably be to call m2.ssl_read again (until it returns someting != None).)

+ """ + + def __init__(self, *args, **kwargs): + # set term_width to avoid auto-detection in progressbar's code + # that sets SIGWINCH signal handler + kwargs["term_width"] = self._get_term_width() + super(ProgressBar, self).__init__(*args, **kwargs) + + def _get_term_width(self): + try: + _, width = array.array('h', fcntl.ioctl(sys.stderr, termios.TIOCGWINSZ, '\0' * 8))[:2]

Hmm the winsize struct consists of unsigned short members. Strictly speaking, we should use "H" instead of "h". We could also use struct.calcsize to compute the "correct" size of the winsize struct size = struct.calcsize('4H') winsize = fcntl.ioctl(sys.stderr, termios.TIOCGWINSZ, '\0' * size) width = struct.unpack('4H', winsize)[1] (just an idea...)

+ except Exception:

Hmm what about only catching OSError?

+ width = self._env_size() + return width + + def update(self, *args, **kwargs): + # recompute term_width in case the term width has changed + self.term_width = self._get_term_width()

Hmm we probably end up with "a lot" of unnecessary ioctl syscalls:/ but I guess that's OK... Otherwise, nice patch!:)

[dmach]

@marcus-h all right, after reading your comment I've started digging in how signals work in Python and realized, that there could be a way simpler patch. Please check the new version and let me know if you have any concerns.

[marcus-h]

On 2022-01-03 02:52:24 -0800, Daniel Mach wrote: @marcus-h all right, after reading your comment I've started digging in how signals work in Python and realized, that there could be a way simpler patch. Please check the new version and let me know if you have any concerns.

Cool! LGTM. Minor: while you are at it, what about removing the (useless) SIGWINCH stuff from the babysitter? (Feel free to ignore:) )

[dmach]

I've removed the useless SIGWINCH stuff in another commit in this PR.
Just in case, I've manually tested that osc works in quiet mode as expected (a different code path than the default SIGWINCH handling).

[marcus-h]

On 2022-01-07 12:55:02 +0000, Daniel Mach wrote: I've removed the useless SIGWINCH stuff in another commit in this PR. Just in case, I've manually tested that osc works in quiet mode as expected (a different code path than the default SIGWINCH handling).

Merged. Thanks a lot!