savannah: consecutive resize events not handled correctly #117
Comments
(In #2198) Replying to Spinal (#2198):
Definetely, no.
Strange. I've tried 4.6.1 from repo, and it doesn't work also. It seems, subshell resizing in NCurses-based mc didn't work ever.
Replying to egmont:
Correct, it is an NCurses-related bug only. But implementation of window resize handling in S-Lang-based mc is not quite correct.
In general, the implementation of SIGWINCH signal handling in mc should be rewritten. One of possible way is described in #117. |
|
|
|
i had a quick look at the code, and it is the very definition of "mess". :)
what should be used here is the so-called signal self-pipe trick which is employed to serialize signals into the regular event loop.
that would also remove the need for "minimal" resize handling and special-casing of slang vs. ncurses. the current code isn't signal-safe anyway - you may still get a random crash during resizing.
the same also applies to the sigchld handler (or rather, the two of them, which is a problem in itself). so maybe it would be appropriate to re-purpose this bug to a general "clean up signal handling". |
|
|
to fix this bug properly, one needs to make some improvements to the main loop. |
Replying to ossi:
Yes, we think about that. |
|
|
Now that pselect() is widely available, one quick and easy way to properly fix the problem is to change the main select() call to a pselect() which blocks/unblocks sigwinch. Glib mainloop would of course be much nicer, but probably more work. For what it's worth, the bug never occurred to me since it was "almost fixed", the race condition window is so small. |
i don't really like that - the critical sections would have to be quite large, and it's a nightmare to make sure everything is covered.
note that the glib mainloop is no hard precondition. i just wouldn't want to think of hacking the existing mainloop(s) if a cleaner solution is planned anyway. |
Branch: 117_sigwinch |
Is it really worth it? Looks quite complex for a nasty hackish workaround to me, instead of using the proper pselect or ppoll. |
Also, are these pipe fds closed when launching any external process? I can't test the patch now, sorry, but there's at least no cloexec in it. |
signal serialization via a pipe is the standard way to do things in an event loop based application. indeed, the linux kernel recently added dedicated poll()-able objects to make the workaround with the pipe unnecessary (forkfd, signalfd, eventfd, and some more iirc).
this implementation has the additional feature of signal compression, which is appropriate.
leaked fds should be indeed avoided. see pipe2() and fcntl() with O_CLOEXEC (of course, one can manually track the fds as well, but that's annoying). |
fwiw, the error handling should be a lot less defensive - when pipe() or fcntl() fail, something is seriously wrong, and you should perror() and exit(3) rather than falling back to no action. that also removes the need for conditionals down the path. |
I'd agree that this used to be the standard way (or rather: standard workaround) for this, ages ago, until a cleaner solution was designed.
According to its manual page, "pselect() is defined in POSIX.1g, and in POSIX.1-2001 and POSIX.1-2008", furthermore, "pselect() was added to Linux in kernel 2.6.16" (which was released in 2006). That's not "recently", that's a long ago. |
pselect() has portability problems. |
Released in 2005
2010
2001
2003
2006
2000
2002
last release is 6 years old
2015 (if I'm looking at the right thing: Visual C++ 2015 a.k.a. C++ 14.0), two new versions since
2004
last version in 2000
2013
Honestly, why do we care about such old systems, or ones that haven't been able to implement a standard for 10+ (but even like 18-ish) years?
Let me put it into a different perspective: Is there any platform where mc currently compiles and runs fine, and pselect would break it? |
how exactly is pselect() a pollable object? i don't think you bothered to research the relevant keywords i posted here ...
even with perfect pselect() support, getting signal handling right in an event-loop based application (and even more a library, like glib and qt) is basically an impossible task. just forget about it. |
Indeed, my bad, I didn't literally answer to your question, rather meant to answer to the generic concerns about pselect.
Could you please elaborate? Especially in the context of mc, which isn't a library, doesn't use glib's main loop, nor qt.
mc spends its idle times in one of its few select() calls. Let's generally block WINCH, and replace these select()s with pselect()s that unblock WINCH for their duration only; let the signal handler flip a bit (as it does now), and let's check for that bit after the pselect() and resize if necessary (as it's pretty much done now). What's the problem?
Given that pselect was specifically designed to solve this very problem we're arguing about, e.g. quoting from its manual page
I'm truly skeptical if anyone says pselect() is a problematic choice. |
Replying to egmont:
as a matter of fact, it's supposed to start using the glib main loop once someone does the work. the current loops are an utter #ifdef mess.
"one of its few" is a recipe for "fun".
none so far, except that you get an extremely rigid structure that gets harder to extend and more fragile each time you (try to) add something to it. sigchld? timers? |
Let's code against current mc, not a future mc that we have in our minds but is unlikely to get implemented in the foreseeable future.
The very same kind and amount of "fun" as the current raceable WINCH handling, or the proposed self-pipe solution.
I truly don't see it justified that replacing the current select() with a pselect() to temporarily unblock WINCH would result in an any more rigid structure than the self-pipe trick, e.g. I don't see it justified that adding a timer to pselect() would be any more complicated than adding it to select().
Mind you, it doesn't hold in the other direction either. It's not that the self-pipe trick is bad per se, nor that it's significantly more complex than pselect. It's just that the self-pipe trick is an ancient workaround for something that has had a properly designed solution for 10+ years. Hence my preference of going with the latter. |
Replying to egmont:
the code is already written and happens to be more compatible with the assumed future direction. it would be plain silly to change the approach now. |
Including the lack of O_CLOEXEC which needs to be fixed before merging to master.
I still don't see this claim being justified.
Let andrew_b pick whichever of these two approaches, based on the comments so far. Andrew, whichever you pick, thanks for the work! |
Sorry, I'm too busy and have no time to fix my code. I'll to that on weekend. |
Fixed: [3b124b0d058afbb223264dd8217b4224ae9a4c38]. |
the pipe handling is now correct afaict.
anyway, i had a somewhat closer look at the rest now, and i'm not sure it makes sense: why is the setup specific to the toolkit? why are you still chaining to slang? isn't it possible to notify it externally, synchronously with the main loop?
the other question i have is how this plays with nested main loops (i.e., modal dialogs). will the whole stack resize, or only the top window? |
Replying to ossi:
Because we have two different toolkits.
What do you mean? I'm afraid I'm not quite understanding you.
pipe is just replacement for global flag winch_flag. SIGWINCH handling is unchanged, |
Replying to andrew_b:
that doesn't answer the question. if the setup was toolkit-agnostic, there would be no need to treat them separately.
the slang codepath still passes on sigwinch directly to slang. it is well possible that slang offers an api function to trigger a size adjustment, making the special-cased signal handling unnecessary.
there is a major difference, which is that now querying the pipe implicitly resets the "flag", while previously it didn't. and the querying is done in several places. i didn't check the implications, just pointing it out. |
Replying to ossi:
It wasn't.
Unfortunately, no. |
|
|
Important
This issue was migrated from Trac:
egmont(@egmontkob)ossi(@ossilator),zaytsev(@zyv),egmont@….com(@egmontkob)Original: http://savannah.gnu.org/bugs/?17822
Original submission:
Comment 1 by Egmont Koblinger <egmont> at Tue 10 Oct 2006 03:27:46 PM UTC:
Comment 2 by Egmont Koblinger <egmont> at Tue 10 Oct 2006 03:40:51 PM UTC:
Comment 3 by Egmont Koblinger <egmont> at Tue 10 Oct 2006 04:22:16 PM UTC:
Comment 4 by Leonard den Ottolander <leonardjo> at Fri 03 Nov 2006 07:14:55 PM UTC:
Comment 5 by Egmont Koblinger <egmont> at Mon 06 Nov 2006 12:01:58 PM UTC:
Comment 6 by Leonard den Ottolander <leonardjo> at Mon 06 Nov 2006 10:24:35 PM UTC:
Comment 7 by Leonard den Ottolander <leonardjo> at Wed 08 Nov 2006 01:38:24 PM UTC:
Comment 8 by Denis Vlasenko <vda> at Mon 01 Oct 2007 03:21:44 PM UTC:
Comment 9 by Oswald Buddenhagen <ossi> at Mon 01 Oct 2007 09:03:01 PM UTC:
Comment 10 by Pavel Tsekov <ptsekov> at Tue 02 Oct 2007 09:18:19 AM UTC:
Note
Original attachments:
slavazanko(@slavaz) onJan 2, 2009 at 21:26 UTCThe text was updated successfully, but these errors were encountered: