inputready_fd has become unreliable

[kmarius]

Since the recent changes to input handling I noticed that inputready_fd became unreliable. I made a small example that can be compiled with gcc input.c -lnotcurses -lnotcurses-core here: https://gist.github.com/kmarius/cf12ceba809c9dd402fdf596c2a164be

When pressing (different) keys sometimes poll does not show readiness and instead on a following keypress all input is processed. Also try holding down one key.

In my application I use a libev event loop which shows the same behavior.

Please include the following data:

COLORTERM truecolor
LANG en_US.UTF-8
LESS_TERMCAP_mb \e\[01\;31m
LESS_TERMCAP_md \e\[01\;31m
LESS_TERMCAP_me \e\[0m
LESS_TERMCAP_se \e\[0m
LESS_TERMCAP_so \e\[01\;44\;33m
LESS_TERMCAP_ue \e\[0m
LESS_TERMCAP_us \e\[01\;32m
TERM foot
TERMINAL /usr/bin/foot

• notcurses 2.4.2
• foot version: 1.9.0 +pgo +ime +graphemes. I also tried tmux, st, xterm.

[dankamongmen]

wow, someone's using that functionality! man, i need to go ahead and get more rigorous about changing semantics out. hopefully this input overhaul will be one of the last major surgeries.

i thought about this the other day, actually. we now might be reading from two fds, but only one would ever be relevant to the user. the problem is that it's no longer enough to poll() on that fd, since i might have already cleared it of input. the only thing i can see working is give you a different file descriptor (the contract on notcurses_inputready_fd() allows this:

// Get a file descriptor suitable for input event poll()ing. When this descriptor becomes available, you can call notcurses_getc_nblock(), and input ought be ready. This file descriptor is not necessarily the file descriptor associated with stdin (but it might be!)

and then i can hit that fd with a write whenever i've got something buffered. that part isn't too tough; the problem is how i would set this file descriptor unreadable when you collect your input. i'm sure it can be done, or at least suspect so.

either way, thanks for bringing it to my attention that someone's using this. my humble apologies for the annoyance; i'll try my best to have it resolved by morning.

[dankamongmen]

and to you and anyone reading this: i'm sorry about how this input transition has gone, and embarrassed. i could and ought have handled it a lot better. the work is absolutely necessary, but it ought have been kept away from the public releases until fully rigorous. i will do better in the future.

[dankamongmen]

i'll look into draining a pipe from the write end; if that's possible, this will be trivial. otherwise, perhaps i can read from the pipe after handing out all your input; my only worry there would be potential interactions with some other user thread. that needs be carefully analyzed, should we go down that route. but it ought be workable.

yeah, you can expect a fix tonight i think.

[dankamongmen]

yeah, i'm a superdumbass, that second method will work just fine, and portably. good good.

[dankamongmen]

yeah i ought be able to hack this together in about a half hour once i emerge from the dayjob's code mines, fear not

[kmarius]

Since this issue is now open I would also like mention that something like system("bash") doesn't seem to work at all. The program exits on the first keystroke.

[dankamongmen]

Since this issue is now open I would also like mention that something like system("bash") doesn't seem to work at all. The program exits on the first keystroke.

launching arbitrary subprocesses has never been supported in rendered mode (it's allowed in direct mode). an ncsubproc ought work for you here, though. i've actually never run a subprocess that was handling interactive input from Notcurses; it's something that certainly ought work, but i can't say i've ever investigated it. i'll make another issue for that, if you don't mind, and keep this one about notcurses_inputready_fd().

were you doing things like system("bash") prior to the 2.4.1 input rewrite, and it worked? bah let me ask you in the new bug =]

[dankamongmen]

the issue you bring up with interactive subprocesses is being tracked in #2217.

[dankamongmen]

i'm now creating a pipe pair for the inputctx, and handing out the read end for input readiness checking. i now need to write to it, and clear it.

[dankamongmen]

I've got PR #2218 up for this. It's a simple and naive solution, but it ought work. We'll want to punch it up when I'm not seconds away from collapsing.

[dankamongmen]

things ought work once more. thanks for the report!

[kmarius]

@dankamongmen I was going to update from 2.4.1 to 2.4.9 and I still don't think this works right. With the example progam I linked initially, poll blocks until I press a special key (e.g. arrow keys) and then all input is drained. (And pressing two special keys seems to break it).

[dankamongmen]

thanks for taking another look; i'll check it out this weekend and see if i've done something stupid. sorry for the annoyance!

[dankamongmen]

i whipped up some test code; it's in the tree as src/poc/cli.c:

#include <poll.h>
#include <notcurses/notcurses.h>

int main(void){
  struct notcurses_options nopts = {
    .flags = NCOPTION_PRESERVE_CURSOR |
             NCOPTION_NO_CLEAR_BITMAPS |
             NCOPTION_NO_ALTERNATE_SCREEN,
  };
  struct notcurses* nc = notcurses_init(&nopts, NULL);
  if(nc == NULL){
    return EXIT_FAILURE;
  }
  struct ncplane* stdn = notcurses_stdplane(nc);
  ncplane_set_scrolling(stdn, true);
  ncinput ni;
  int fd = notcurses_inputready_fd(nc);
  do{
    if(ncplane_putstr(stdn, "press any key\n") < 0){
      goto err;
    }
    if(notcurses_render(nc)){
      goto err;
    }
    struct pollfd pfd = {
      .fd = fd,
      .events = POLLIN,
    };
    while(poll(&pfd, 1, -1) <= 0){
    }
    notcurses_get_blocking(nc, &ni);
  }while(ni.evtype == NCTYPE_RELEASE || ni.id != 'q');
  if(notcurses_render(nc)){
    goto err;
  }
  notcurses_stop(nc);
  return EXIT_SUCCESS;

err:
  notcurses_stop(nc);
  return EXIT_FAILURE;
}

it seems to be working perfectly. let me take a look at the code you provided and see what's up.

[dankamongmen]

i've taken your code and updated it for current trunk (pre-3.0.0), very minor changes:

// gcc input.c -lnotcurses -lnotcurses-core                                                                                         
                                                                                                                                    
#include <notcurses/notcurses.h>                                                                                                    
#include <poll.h>                                                                                                                   
                                                                                                                                    
struct notcurses *nc;                                                                                                               
struct ncplane *n;                                                                                                                  
                                                                                                                                    
int main() {                                                                                                                        
  int ready;                                                                                                                        
  ncinput in;                                                                                                                       
                                                                                                                                    
  notcurses_options opt = {};                                                                                                       
  nc = notcurses_core_init(&opt, NULL);                                                                                             
                                                                                                                                    
  unsigned ncols, nrows;                                                                                                            
  notcurses_stddim_yx(nc, &nrows, &ncols);                                                                                          
  struct ncplane_options opts = {                                                                                                   
    .cols = ncols,                                                                                                                  
    .rows = nrows,                                                                                                                  
    .x = 0,                                                                                                                         
    .y = 0,                                                                                                                         
  };                                                                                                                                
                                                                                                                                    
  n = ncplane_create(notcurses_stdplane(nc), &opts);                                                                                
  notcurses_render(nc);                                                                                                             
                                                                                                                                    
  struct pollfd pfd = {                                                                                                             
    .fd = notcurses_inputready_fd(nc),                                                                                              
    .events = POLLIN,                                                                                                               
  };                                                                                                                                
                                                                                                                                    
  while (1) {                                                                                                                       
    ready = poll(&pfd, 1, -1);                                                                                                      
    if (ready == -1)                                                                                                                
      return 1;                                                                                                                     
    if (pfd.revents & POLLIN) {                                                                                                     
      int i = 0;                                                                                                                    
      ncplane_erase(n);                                                                                                             
      while (notcurses_get_nblock(nc, &in)) {                                                                                       
        ncplane_printf_yx(n, i++, 0, "%u", in.id);                                                                                  
      }                                                                                                                             
      notcurses_render(nc);                                                                                                         
    }                                                                                                                               
  }                                                                                                                                 
                                                                                                                                    
  notcurses_stop(nc);                                                                                                               
  return 0;                                                                                                                         
} 

[dankamongmen]

agreed that yours is returning POLLIN when it shouldn't be. debugging it now.

[dankamongmen]

[pid 3978964] ppoll([{fd=0, events=POLLIN|POLLRDHUP}],

strace on your binary shows the poll being run on fd 0, which seems wrong...

[dankamongmen]

and yet when i print it, we clearly get 4...

[dankamongmen]

ahhh, i think i see the error. thanks for the great reproduction case! testing fix now...

[dankamongmen]

yep! when there was a single character pending, we didn't clear the eventready fd, but we always need to on successful return. i believe it's now fixed. your code runs as i believe it is intended to.

this fix is in master, which is the pre-3.0 trunk. like i said, your code above required very minimal adaptation. let me know if you need any assistance.

thanks a lot for staying diligently on top of this!

[kmarius]

I have tried the newest release and I am not seeing any difference from what I had described here:

@dankamongmen I was going to update from 2.4.1 to 2.4.9 and I still don't think this works right. With the example progam I linked initially, poll blocks until I press a special key (e.g. arrow keys) and then all input is drained. (And pressing two special keys seems to break it).

[dankamongmen]

with recent XTMODKEYS changes, we're seeing:

[pid 1799954] read(0, "\r", 8192)       = 1
[pid 1799954] ppoll([{fd=0, events=POLLIN|POLLRDHUP}], 1, NULL, ~[CONT WINCH RTMIN RT_1], 8) = 1 ([{fd=0, revents=POLLIN}])
[pid 1799954] read(0, "\r", 8192)       = 1
[pid 1799954] ppoll([{fd=0, events=POLLIN|POLLRDHUP}], 1, NULL, ~[CONT WINCH RTMIN RT_1], 8) = 1 ([{fd=0, revents=POLLIN}])
[pid 1799954] read(0, "\r", 8192)       = 1
[pid 1799954] ppoll([{fd=0, events=POLLIN|POLLRDHUP}], 1, NULL, ~[CONT WINCH RTMIN RT_1], 8) = 1 ([{fd=0, revents=POLLIN}])
[pid 1799954] read(0, "\r", 8192)       = 1
[pid 1799954] ppoll([{fd=0, events=POLLIN|POLLRDHUP}], 1, NULL, ~[CONT WINCH RTMIN RT_1], 8) = 1 ([{fd=0, revents=POLLIN}])
[pid 1799954] read(0, "\r", 8192)       = 1
[pid 1799954] ppoll([{fd=0, events=POLLIN|POLLRDHUP}], 1, NULL, ~[CONT WINCH RTMIN RT_1], 8) = 1 ([{fd=0, revents=POLLIN}])
[pid 1799954] read(0, "a", 8192)        = 1
[pid 1799954] ppoll([{fd=0, events=POLLIN|POLLRDHUP}], 1, NULL, ~[CONT WINCH RTMIN RT_1], 8

for common inputs, but if i throw a ctrl in there, we instead get:

[pid 1799954] read(0, "a", 8192)        = 1
[pid 1799954] ppoll([{fd=0, events=POLLIN|POLLRDHUP}], 1, NULL, ~[CONT WINCH RTMIN RT_1], 8) = 1 ([{fd=0, revents=POLLIN}])
[pid 1799954] read(0, "a", 8192)        = 1
[pid 1799954] ppoll([{fd=0, events=POLLIN|POLLRDHUP}], 1, NULL, ~[CONT WINCH RTMIN RT_1], 8) = 1 ([{fd=0, revents=POLLIN}])
[pid 1799954] read(0, "\33[27;5;13~", 8192) = 10
[pid 1799954] write(5, "\1", 1 <unfinished ...>
[pid 1799952] <... poll resumed>)       = 1 ([{fd=4, revents=POLLIN}])
[pid 1799954] <... write resumed>)      = 1
[pid 1799952] futex(0x629000004288, FUTEX_WAIT_PRIVATE, 2, NULL <unfinished ...>
[pid 1799954] futex(0x629000004288, FUTEX_WAKE_PRIVATE, 1 <unfinished ...>
[pid 1799952] <... futex resumed>)      = -1 EAGAIN (Resource temporarily unavailable)
[pid 1799954] <... futex resumed>)      = 0
[pid 1799952] read(4,  <unfinished ...>
[pid 1799954] futex(0x629000004288, FUTEX_WAIT_PRIVATE, 2, NULL <unfinished ...>
[pid 1799952] <... read resumed>"\1", 1) = 1
[pid 1799952] read(4, 0x7ffea7458ab0, 1) = -1 EAGAIN (Resource temporarily unavailable)
[pid 1799952] futex(0x629000004288, FUTEX_WAKE_PRIVATE, 1) = 1
[pid 1799954] <... futex resumed>)      = 0
[pid 1799954] futex(0x629000004288, FUTEX_WAKE_PRIVATE, 1) = 0
[pid 1799952] ioctl(3, TIOCGWINSZ <unfinished ...>
[pid 1799954] ppoll([{fd=0, events=POLLIN|POLLRDHUP}], 1, NULL, ~[CONT WINCH RTMIN RT_1], 8 <unfinished ...>
[pid 1799952] <... ioctl resumed>, {ws_row=61, ws_col=80, ws_xpixel=884, ws_ypixel=1415}) = 0
[pid 1799952] rt_sigprocmask(SIG_BLOCK, [INT QUIT TERM], [CONT WINCH], 8) = 0
press any key, q to quit\33[62;1H\n\33[60;1Hpress any key, q "..., 47
 ) = 47
[pid 1799952] rt_sigprocmask(SIG_SETMASK, [CONT WINCH], NULL, 8) = 0
)     = 4952] write(1, "\33[1G", 4
[pid 1799952] poll([{fd=4, events=POLLIN}], 1, -1

so why are we writing to the pipe in this latter case, but not in the former?

[dankamongmen]

are we somehow not calling through load_ncinput()?

[dankamongmen]

i don't think we are!!! but we are for kitty! Vancouver, Vancouver, this is it!

[dankamongmen]

that's absolutely it, that's what's going on here

[dankamongmen]

YEP. in process_ncinput(), we're directly controlling the condition var...BUT NOT THE PIPE.

  pthread_mutex_unlock(&ictx->ilock);                                                                                               
  pthread_cond_broadcast(&ictx->icond);     

that's absolutely got to be it.

[dankamongmen]

aye, that appears to fix it! woo-hah!

[dankamongmen]

ok that definitely fixes the cli1 problem i was seeing in non-kitty terminals as described in #2537. let me try out your original code @kmarius , but i bet this got it (finally)!

[dankamongmen]

hrmmm, i'm not certain it's fixed the original issue, erf

[dankamongmen]

oh, whoops, i was linking with my system copy rather than my local build; i do believe this is fixed!

[dankamongmen]

yeah @kmarius thanks for your long-suffering patience, but i think this one is down.