Re: callback api, isearch, signals issue

[Andrew Burgess]

Chet Ramey <chet.ramey@case.edu> writes:

> On 1/9/23 8:59 AM, Andrew Burgess wrote:
>
>> A grep on the GDB source tree indicates we don't set
>> rl_signal_event_hook anywhere.  I'll take a look now at the role that
>> plays in this process and see if it will help at all, but I'd also
>> welcome any insights you have.
>
> The idea of the event hook is an application-specific event handler that
> can be called after read is interrupted by a signal. The application is
> the `agent' that has to decide what the effect of the signal should be.
> To use bash as an example -- which does not use the callback interface --
> it's the difference between what you do when SIGINT is trapped and when
> it's set to the default disposition. Readline is agnostic in that regard.
>
> So we're back to mismatched expectations. Readline is expecting the
> application to abort the operation, and gdb expects readline to enforce
> some behavior internally. We have a policy issue.
>
> It's pretty clear that the desired behavior, at least for the purposes of
> this report, here in the case of SIGINT during `stateful' operations like
> search or digit-argument input is to abort, in the absence of any other
> guidance from the application.
>
> The question is who makes that decision, or at least enforces it. Should
> readline impose some sort of default rl_abort behavior in the case that
> application doesn't? That's easy enough to do in the signal cleanup path.
> Or should it leave it completely up to the application to decide when to
> call rl_abort or some other return to the top-levep REPL?

I'm afraid that I didn't really understand this explanation.

I tried setting up the event handler in the demo application (that I
sent in the first email of this thread), and I don't believe that the
callback is ever invoked before readline crashes in the example I gave.

You talk about an expectation mismatch between readline and the
application using readline, except, in the example I gave, it doesn't
seem that the application is involved at all, this appears to be
entirely an internal readline issue; the SIGINT arrives, readline
cancels the isearch, then readline calls rl_isearch_dispatch.

I'm wondering if _rl_isearch_dispatch should be extended to check if the
isearch has already been aborted?  I made this change and then found
that my test program didn't work exactly like GDB.  Below you will find
my (straw-man) readline patch, along with an updated test program.

The steps to use the test program are as before:

  1. Compile and run the test program,
  2. Press ctrl-r to enter reverse-i-search mode,
  3. Press any "normal" key, e.g. 'a', this triggers the SIGINT.
  4. With a patched readline you'll see a 'In sigint_handler' message,
     followed by a 'Quit', followed by a redisplayed prompt.
  5. You can now interact with readline normally.

Thanks,
Andrew

---

diff --git a/isearch.c b/isearch.c
index c2d4d23..12c2175 100644
--- a/isearch.c
+++ b/isearch.c
@@ -346,6 +346,11 @@ _rl_isearch_dispatch (_rl_search_cxt *cxt, int c)
   int j;
   rl_command_func_t *f;
 
+  /* If we got a signal then isearch mode may already have been cleaned
+     up.  */
+  if (!RL_ISSTATE(RL_STATE_ISEARCH))
+    return -1;
+
   f = (rl_command_func_t *)NULL;
 
   if (c < 0)

---

/* Standard include files. stdio.h is required. */
#include <errno.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <locale.h>

/* Used for select(2) */
#include <sys/types.h>
#include <sys/select.h>

#include <signal.h>

#include <stdio.h>

/* Standard readline include files. */
#include <readline/readline.h>
#include <readline/history.h>

static void cb_linehandler (char *);
static void sigwinch_handler (int);
static void sigint_handler (int);

int running;
int sigwinch_received;
int sigint_received;
const char *prompt = "rltest$ ";

/* Used to signal a SIGINT has arrived.  */
int pipe_fds[2];

/* Handle SIGWINCH and window size changes when readline is not active and
   reading a character. */
static void
sigwinch_handler (int sig)
{
  sigwinch_received = 1;
}

/* Handle SIGWINCH and window size changes when readline is not active and
   reading a character. */
static void
sigint_handler (int sig)
{
  char c;

  fflush (stdout);
  printf ("In sigint_handler\n");
  fflush (stdout);
  sigint_received = 1;

  c = 'X';
  write (pipe_fds[1], &c, 1);
}

/* Callback function called for each line when accept-line executed, EOF
   seen, or EOF character read.  This sets a flag and returns; it could
   also call exit(3). */
static void
cb_linehandler (char *line)
{
  /* Can use ^D (stty eof) or `exit' to exit. */
  if (line == NULL || strcmp (line, "exit") == 0)
    {
      if (line == 0)
        printf ("\n");
      printf ("exit\n");
      /* This function needs to be called to reset the terminal settings,
         and calling it from the line handler keeps one extra prompt from
         being displayed. */
      rl_callback_handler_remove ();

      running = 0;
    }
  else
    {
      if (*line)
        add_history (line);
      printf ("input line: %s\n", line);
      free (line);
    }
}

int count = 2;

static int
my_getc (FILE *stream)
{
  if (--count == 0)
    {
      kill (getpid (), SIGINT);
    }

  int ch = rl_getc (stream);

  return ch;
}

static int
my_signal_event_hook (void)
{
  fflush (stdout);
  printf ("In my_signal_event_hook\n");
  fflush (stdout);
}

int
main (int c, char **v)
{
  fd_set fds;
  int r;

  /* Set the default locale values according to environment variables. */
  setlocale (LC_ALL, "");

  /* Handle window size changes when readline is not active and reading
     characters. */
  signal (SIGWINCH, sigwinch_handler);

  signal (SIGINT, sigint_handler);

  rl_getc_function = my_getc;

  rl_signal_event_hook = my_signal_event_hook;

  /* Install the line handler. */
  rl_callback_handler_install (prompt, cb_linehandler);

  if (pipe (pipe_fds) < 0)
    abort ();

  /* Enter a simple event loop.  This waits until something is available
     to read on readline's input stream (defaults to standard input) and
     calls the builtin character read callback to read it.  It does not
     have to modify the user's terminal settings. */
  running = 1;
  while (running)
    {
      FD_ZERO (&fds);
      FD_SET (fileno (rl_instream), &fds);
      FD_SET (pipe_fds[0], &fds);

      r = select (FD_SETSIZE, &fds, NULL, NULL, NULL);
      if (r < 0 && errno != EINTR)
        {
          perror ("rltest: select");
          rl_callback_handler_remove ();
          break;
        }
      if (sigwinch_received)
        {
          rl_resize_terminal ();
          sigwinch_received = 0;
        }
      if (sigint_received)
        {
          char c;
          if (read (pipe_fds[0], &c, 1) != 1)
            abort ();
          printf ("Quit\n");
          fflush (stdout);

          rl_callback_handler_remove ();
          rl_callback_handler_install (prompt, cb_linehandler);

          sigint_received = 0;
          continue;
        }
      if (r < 0)
        continue;

      if (FD_ISSET (fileno (rl_instream), &fds))
        rl_callback_read_char ();
    }

  printf ("rltest: Event loop has exited\n");
  return 0;
}

---