Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit

[Eli Zaretskii <eliz@gnu.org>]

> Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 30 Jan 2017 13:52:46 -0800
> 
>     I don't understand why we need both this and
>     emacs_read, which cannot be interrupted.  Why not have just emacs_read
>     which can be interrupted, and use that all over?
> 
> For example, filelock.c's read_lock_data opens a file, uses emacs_read to read it, and then closes the file. If read_lock_data used emacs_read_quit it might process a quit, which would skip the close and leak a file descriptor.
> 
> The read_lock_data issue could be fixed by having it call record_unwind_protect_int (close_file_unwind, fd) before calling emacs_read. Possibly all these dicey uses of emacs_read could be fixed in a similar way. However, that would be a bigger and more-intrusive change, and in the read_lock_data case it arguably would be overkill and I wanted to keep the patch smaller. I used emacs_read_quit only in places that I verified were safe, and stuck with emacs_read when I wasn't sure, or where more-intrusive changes would be needed.

I indeed think that we should make emacs_read support quitting, and
add unwind_protect calls where we currently don't.  This should be
safer in the long run, and also simpler.  As for overhead, operations
like locking a file should indeed normally be very fast, but could
take perceptible time in some exceptional conditions, like networked
volumes or high I/O load, in which case users may wish to interrupt
that.

But yes, this could be done as a separate changeset.

>        @@ -1252,6 +1256,7 @@ search_buffer (Lisp_Object string, ptrdiff_t pos, 
>     ptrdiff_t pos_byte,
>                     return (n);
>                   }
>                 n++;
>        +      maybe_quit ();
>               }
>              while (n > 0)
>               {

>     regex.c calls maybe_quit internally, so why do we need this additional
>     call?
> 
> The regex code does not always call maybe_quit. For example, without this additional call, (re-search-forward "[[:alpha:]]" nil nil most-positive-fixnum) would loop indefinitely in a buffer containing only alphabetic characters on a 64-bit platform.

Then maybe we should add maybe_quit calls in regex.c instead?
Currently, immediate_quit is non-zero all the time re_search_2 runs,
so on a TTY a C-g will can stop regex.c in its tracks anywhere.  I
thought we wanted to make GUI sessions as responsive as TTY sessions.

>        @@ -724,6 +725,8 @@ back_comment (ptrdiff_t from, ptrdiff_t from_byte, 
>     ptrdiff_t stop,
>             that determines quote parity to the comment-end.  */
>           while (from != stop)
>             {
>        +      incr_rarely_quit (&quit_count);
>        +
> 
>     Is it safe to quit from back_comment?  It manipulates a global
>     variable gl_state, and I don't see unwind-protect calls anywhere in
>     sight.
> 
> It should be OK. The current master sets immediate_quit=true in back_comment's callers (both scan_lists and Fforward_comment), so current master already lets back_comment quit.

Yes, but that is why we have gl_state-related dance in
handle_interrupt, and your changes delete that part.

> If Emacs quits in back_comment, it should longjmp to code that reinitializes gl_state before using it.

But unwinding the Lisp stack could run some Lisp that uses syntax.c
functions, before we longjmp, right?

> This also applies to the other places you mentioned. The idea is to insert maybe_quit calls in code that was already subject to immediate_quit=true in the current master, so it should be safe to quit.

That assumes all the immediate_quit=true settings were safe.
Previously, they were only in effect on TTY frames, whereas now the
maybe_quit calls will be in effect everywhere, so their exposure to
various use cases will be much wider.  That's why I think it's prudent
to take a good look at these places while we make these changes.

But I don't feel I know enough about this aspect of syntax.c.  Stefan,
can you comment on this, please?

>        @@ -10445,30 +10433,12 @@ handle_interrupt (bool in_signal_handler)
>             }
>           else
>             {
>        -      /* If executing a function that wants to be interrupted out of
>        -     and the user has not deferred quitting by binding `inhibit-quit'
>        -     then quit right away.  */
>        -      if (immediate_quit && NILP (Vinhibit_quit))
>        -    {
>        -      struct gl_state_s saved;
>        -
>        -      immediate_quit = false;
>        -      pthread_sigmask (SIG_SETMASK, &empty_mask, 0);
>        -      saved = gl_state;
>        -      quit ();
>        -      gl_state = saved;
>        -    }
>        -      else
>        -        { /* Else request quit when it's safe.  */
>        -      int count = NILP (Vquit_flag) ? 1 : force_quit_count + 1;
>        -      force_quit_count = count;
>        -      if (count == 3)
>        -            {
>        -              immediate_quit = true;
>        -              Vinhibit_quit = Qnil;
>        -            }
>        -          Vquit_flag = Qt;
>        -        }
>        +      /* Request quit when it's safe.  */
>        +      int count = NILP (Vquit_flag) ? 1 : force_quit_count + 1;
>        +      force_quit_count = count;
>        +      if (count == 3)
>        +    Vinhibit_quit = Qnil;
>        +      Vquit_flag = Qt;
>             }
> 
>     This loses the feature whereby C-g on a TTY would quit much faster.
>     Why is this a good idea?
> 
> Speed is not a problem, as C-g (with the proposed changes) should quit just as fast on a TTY as it already does under X, and it's been working that way under X for some time.

No, it will be slower.  A signal handler will always react faster than
any solution based on polling.  A signal handler is also capable of
interrupting calls to standard C library functions.

It is true that we already have this issue on GUI frames, but I still
feel uneasy about losing this feature.  TTY frames are still quite
popular, even today, in particular for remote sessions.

What do others think about this?

>     And if it is a good idea, why do we still
>     generate SIGINT on C-g (and force GDB to handle SIGINT specially to
>     support that)?
> 
> Inertia, I think. Having C-g generate SIGINT made sense when we had immediate_quit. I expect that it is a useless appendage now, and that in a later patch we can change Emacs so that C-g no longer generates SIGINT but is instead processed like any other input character. 

No, I don't think we can remove the SIGINT generation: if we do, there
will be nothing to set the quit-flag on TTY frames.  Also, the
"emergency exit" feature is also based on SIGINT.

The new patches still include both rarely_quit and incr_rarely_quit
(in the second patchset), which I thought you decided to remove.  Did
you send the correct patches?

Thanks.