Re: [PATCH] Fix the processing of Meta-key commands in TUI

[Patrick Palka <patrick at parcs dot ath dot cx>]

On Thu, Aug 28, 2014 at 7:22 AM, Pedro Alves <palves@redhat.com> wrote:
> On 08/27/2014 07:25 PM, Patrick Palka wrote:
>> On Wed, 27 Aug 2014, Pedro Alves wrote:
>
>>> The main reason I think there's a larger problem here, is that
>>> if curses is reading more than one char from stdin, then that means
>>> that is must be blocking for a bit waiting for the next character,
>>> which is a no-no in an async world, where we want to be processing
>>> target events at the same time.  The man page says:
>>>
>>>  While interpreting an input escape sequence, wgetch sets a timer while waiting for the next character.  If notimeout(win, TRUE) is called, then wgetch does not
>>>  set a timer.  The purpose of the timeout is to differentiate between sequences received from a function key and those typed by a user.
>>>
>>> Looks like there's a default timeout of 1 second.  Indeed if I set a
>>> breakpoint in wgetch and another right after wgetch is called, and
>>> then I press escape, I see that gdb is stuck inside wgetch for around
>>> one second.  During that time, gdb's own event loop isn't being processed.
>>>
>>> Not sure exactly how this is usually handled.  Seems like there
>>> are several knobs that might be able to turn this delay off.
>>> Sounds like we should enable that (whatever the option is),
>>> and handle the timeout ourselves?
>>
>> I don't think the timeout is the issue here.  Even if the timeout is
>> disabled via notimeout(), wgetch() will still attempt to interpret keypad
>> sequences by reading multiple characters from stdin -- except that the
>> read will now be a non-blocking one instead of a blocking one.
>>
>> One way or another, someone must read multiple keys from stdin in order
>> to semantically distinguish between keypad keys and regular key
>> sequences.  And when it turns out that the input is not or cannot be a
>> keypad key then that someone must place the extraneous keys into a
>> buffer and notify GDB's event handler that we missed their stdin events.
>
> Right, that's a given.  What I was talking about is fixing the
> 1 second block in case the input stops before a sequence is complete.
>
>> If we handle the timeout ourselves (for instance by disabling keypad()
>> and enabling notimeout()) then we'll be responsible for doing the
>> lookahead, interpreting the sequences and buffering the keypresses.  I
>> say let ncurses continue to handle the timeout so that we'll only be
>> responsible for notifying the event handler.
>>
>> Though I may just be misunderstanding your proposal.
>
> The main idea was to not let ncurses ever block, as that prevents
> gdb's event loop from handling target events.  If ncurses internally
> already handled the timeout by comparing the time between
> wgetch calls instead of doing a blocking select/poll internally, then
> it'd be a bit easier, but it looks like it doesn't (GetEscdelay always
> starts with the window's configured delay, on each wgetch call?), so we'd
> need to track the timeout ourselves.  Even if it did, it wouldn't be that
> different though.

Is the internal timeout a big deal though?  The handling of the target
event just gets temporarily delayed, not missed entirely, right?  And
isn't this timeout only experienced when one presses ESC (which has no
use in TUI) and/or attempts to manually type a function key sequence?
I'm not sure why anyone would do that.

>
> What we'd need is:
>
>  #1 - set ncurses to be _always_ non-blocking/no-delay.
>  #2 - on input, call wgetch, as today.
>  #3 - due to no-delay, that now only looks ahead for as long as
>       there are already bytes ready to be read from the input file / stdin.
>  #4 - if the sequence looks like a _known_ escape sequence, but
>       it isn't complete yet, then wgetch leaves already-read bytes
>       in the fifo, and returns ERR.  That's the "the keys stay uninterpreted"
>       comment in lib_getch.c.
>  #5 - at this point, we need to wait for either:
>       (a) further input, in case further input finishes the sequence.

Not sure it's possible to make wgetch() behave this way.  From what I
can tell, wgetch() will always return the key from its fifo if there's
one available -- it won't check whether the fifo contents + a new key
from stdin will make a complete sequence.  It won't even read from
stdin if there's a key in the fifo.

>       (b) the timeout to elapse, meaning no further input, and we should
>           pass the raw chars to readline.
>
> For #5/(a), there's nothing to do, that's already what the
> stdin handler does.
>
> For #5/(b), because we don't want to block in the stdin handler (tui_getc)
> blocked waiting for the timeout, we would instead install a timer in gdb's event
> loop whose callback was just be the regular TUI stdin input handler.  This time, given
> enough time had elapsed with no further input, we want the raw chars.  If ncurses
> internally knows that sufficient time has passed, then good, we only have to
> call wgetch, and we know ncurses gives us the raw keys (the incomplete sequence).
> If it doesn't (looks like it doesn't, but I may be wrong), then we just temporarily
> switch off the keypad, and read one char, which returns us the raw escape char at
> the head of the fifo, and leaves the rest in the fifo for subsequent reads.
> As the fifo is now missing the escape char, we can go back to normal, with the
> keypad enabled, and the next time we call wgetch should return us the head of
> the fifo immediately, if there's anything there.
>
> Going back to step #4, in case the sequence is _unknown_ or the timeout
> has expired:
>
>  #4.2 - if the sequence in the fifo is definitely an _unknown_ escape
>     sequence, wgetch returns the first char in the fifo, leaving the
>     remainder of the bytes in the fifo.  TBC, in this case, we _don't_
>     get back ERR.  As there are more bytes in the fifo, then we need
>     to compensate for the missed stdin events, like in your patch.  (*)
>
> (*) - so it sounds your patch would be necessary anyway.
>
> Oddly, even when doing:
>
>   nodelay (w, TRUE);
>   notimeout (w, TRUE);
>
> in tui_getc, I _still_ get that a one second block within wgetch...
> Looks like related to mouse event handling, even though mouse
> events were not enabled...:

Yeah same here.  I can't seem to find the magical invocation that
actually disables this timeout.

>
> (top-gdb) bt
> #0  0x000000373bcea9c0 in __poll_nocancel () at ../sysdeps/unix/syscall-template.S:81
> #1  0x00007f62bc49d43d in _nc_timed_wait (sp=0x18f7730, mode=3, milliseconds=1000, timeleft=0x0) at ../../ncurses/tty/lib_twait.c:265
> #2  0x00007f62bc6bf484 in check_mouse_activity (sp=0x18f7730, delay=1000) at ../../ncurses/base/lib_getch.c:145
> #3  0x00007f62bc6c00c8 in kgetch (sp=0x18f7730) at ../../ncurses/base/lib_getch.c:734
> #4  0x00007f62bc6bfbec in _nc_wgetch (win=0x193e6a0, result=0x7fffdf59f2c8, use_meta=1) at ../../ncurses/base/lib_getch.c:513
> #5  0x00007f62bc6bfe6e in wgetch (win=0x193e6a0) at ../../ncurses/base/lib_getch.c:643
> #6  0x0000000000516d88 in tui_getc (fp=0x373bfb9640 <_IO_2_1_stdin_>) at /home/pedro/gdb/mygit/src/gdb/tui/tui-io.c:662
> #7  0x000000000078385f in rl_read_key () at /home/pedro/gdb/mygit/src/readline/input.c:448
> #8  0x000000000076b5a8 in _rl_subseq_getchar (key=27) at /home/pedro/gdb/mygit/src/readline/readline.c:658
> #9  0x000000000076b5fb in _rl_dispatch_callback (cxt=0x1661190) at /home/pedro/gdb/mygit/src/readline/readline.c:680
> #10 0x0000000000783d73 in rl_callback_read_char () at /home/pedro/gdb/mygit/src/readline/callback.c:170
> #11 0x0000000000618ba9 in rl_callback_read_char_wrapper (client_data=0x0) at /home/pedro/gdb/mygit/src/gdb/event-top.c:167
> #12 0x0000000000618f7f in stdin_event_handler (error=0, client_data=0x0) at /home/pedro/gdb/mygit/src/gdb/event-top.c:373
> #13 0x0000000000617b72 in handle_file_event (data=...) at /home/pedro/gdb/mygit/src/gdb/event-loop.c:763
> #14 0x0000000000617059 in process_event () at /home/pedro/gdb/mygit/src/gdb/event-loop.c:340
> #15 0x0000000000617120 in gdb_do_one_event () at /home/pedro/gdb/mygit/src/gdb/event-loop.c:404
> #16 0x0000000000617170 in start_event_loop () at /home/pedro/gdb/mygit/src/gdb/event-loop.c:429
>
> If such delays/blocks can't be eliminated due to buggy ncurses, or
> something missing in the APIs, then it looks like the only way
> to fix this would be to move the wgetch call to a separate thread,
> like, we'd create a pipe, and put one end in the event loop as
> stdin source instead of the real stdin, and then the separate thread
> would push the results of wgetch into this pipe...

I am not sure that it's possible to eliminate the internal timeout
completely so it seems to me that  your thread-based solution may be
necessary to fix this.  But is it worth the complexity to fix this
seemingly obscure issue?

>
> Thanks,
> Pedro Alves
>