Subject: bug#22493: 25.1.50; open-gnutls-stream doesn't respect :nowait, so the connections are synchronous

From: Eli Zaretskii <[email-masked]>
Date: Tue, 02 Feb 2016 18:10:27 +0200

> From: Lars Ingebrigtsen <[email-masked]>
> Cc: [email-masked]
> Date: Tue, 02 Feb 2016 02:43:02 +0100
> 
> > A sentinel indeed starts when Emacs is idle, but once it kicks in, it
> > runs to completion, and Emacs cannot do anything else while it does.
> > So if the user presses a key a millisecond after the sentinel is
> > invoked and starts the GnuTLS negotiation procedure, the user will
> > still have to wait.
> 
> Sure.  But the wait is much shorter.

If it really is, then I must be missing something, because I don't see
how it could.  Can you show some timings, comparing the "old" and the
"new" methods for making a GnuTLS connection?

And if somehow it indeed is shorter, the same effect can be achieved
by running the GnuTLS negotiation from an idle timer, something that
would most probably avoid most or all of the complexities on the C
level.  Have you considered this alternative?

> Anyway, the next step is, of course, to make the GnuTLS negotiation
> asynchronous, too, so that the entire thing is unnoticeable.
> The library itself is async, but it block because emacs_gnutls_handshake
> does this:
> 
>   do
>     {
>       ret = gnutls_handshake (state);
>       emacs_gnutls_handle_error (state, ret);
>       QUIT;
>     }
>   while (ret < 0 && gnutls_error_is_fatal (ret) == 0);
> 
> Loop until the handshake completes.  The function should instead mark
> the process' status as 'tls-handshake, return and continue the handshake
> from the idle loop.

This will have to be optional, since Emacs might not become idle for a
prolonged time.  IOW, only certain applications will want to benefit
from such a background handshake, some will need to wait for its
completion.  So if you make this be a background thing by default, you
might break existing users that don't expect the negotiation to return
before it's completed or failed.

> Which is something I'm not going to do straight away, because the
> changes already in are complex enough.

Thanks for working on this.  Please allow me some review comments
based on what's on the branch now.

  +@item :tls-parameters
  +When opening a TLS connection, this should be where the first element
  +is the TLS type,

This should tell what values are acceptable for TYPE, since no other
text around this one hints on that.

                   and the remaining elements should form a keyword list
  +acceptable for @code{gnutls-boot}.  The TLS connection will then be
  +negotiated after completing the connection to the host.

This should tell how to obtain those parameters.

  -@defun open-gnutls-stream name buffer host service
  +@defun open-gnutls-stream name buffer host service &optional nowait
   This function creates a buffer connected to a specific @var{host} and
   @var{service} (port number or service name).  The parameters and their
   syntax are the same as those given to @code{open-network-stream}

The meaning of 'nowait' should be explicitly described, since relying
on "the same as those given to 'open-gnutls-stream'" hides some
important information about the handshake in this case.

  -(defun open-gnutls-stream (name buffer host service)
  +(defun open-gnutls-stream (name buffer host service &optional nowait)
     "Open a SSL/TLS connection for a service to a host.
   Returns a subprocess-object to represent the connection.
   Input and output work as for subprocesses; `delete-process' closes it.
  @@ -109,6 +109,8 @@ BUFFER is the buffer (or `buffer-name') to associate with the process.
   Third arg is name of the host to connect to, or its IP address.
   Fourth arg SERVICE is name of the service desired, or an integer
   specifying a port number to connect to.
  +Fifth arg NOWAIT (which is optional) means that the socket should
  +be opened asynchronously.

This last sentence should say more explicitly what happens in that
case.

   It must be omitted, a number, or nil; if omitted or nil it
  -defaults to GNUTLS_VERIFY_ALLOW_X509_V1_CA_CRT."
  +defaults to GNUTLS_VERIFY_ALLOW_X509_V1_CA_CRT.
  +
  +If RETURN-KEYWORDS, don't connect to anything, but just return
  +the computed parameters that we otherwise would be calling
  +gnutls-boot with.  The return value will be a list where the
  +first element is the TLS type, and the rest of the list consists
  +of the keywords."

It's confusing to have a function named "open-gnutls-stream" that
doesn't really open any streams.  So I'd rather have
open-gnutls-stream refactored into 2 functions, with the part that
computes the parameters made a separate function that can be called
directly.  Then both open-gnutls-stream and the async code could call
that new function.

  --- a/lisp/net/network-stream.el
  +++ b/lisp/net/network-stream.el
  @@ -137,7 +137,12 @@ non-nil, is used warn the user if the connection isn't encrypted.
   a greeting from the server.

   :nowait is a boolean that says the connection should be made
  -asynchronously, if possible."
  +asynchronously, if possible.
  +
  +:tls-parameters is a list that should be supplied if you're
  +opening a TLS connection.  The first element is the TLS type, and
  +the remaining elements should be a keyword list accepted by
  +gnutls-boot."

Please mention here how to obtain that keyword list.

  --- a/src/Makefile.in
  +++ b/src/Makefile.in
  @@ -229,6 +229,8 @@ IMAGEMAGICK_CFLAGS= @IMAGEMAGICK_CFLAGS@
   LIBXML2_LIBS = @LIBXML2_LIBS@
   LIBXML2_CFLAGS = @LIBXML2_CFLAGS@

  +GETADDRINFO_A_LIBS = @GETADDRINFO_A_LIBS@

Please add a comment saying when GETADDRINFO_A_LIBS is non-empty.

  +static void
  +boot_error (struct Lisp_Process *p, const char *m, ...)
  +{
  +  va_list ap;
  +  va_start (ap, m);
  +  if (p->is_non_blocking_client)
  +    pset_status (p, Qfailed);
  +  else
  +    verror (m, ap);

AFAIU, here we are losing the error information, which I think we
shouldn't.  Why not keep it around and let the sentinel use it if it
wants to?

  @@ -5665,6 +5901,13 @@ send_process (Lisp_Object proc, const char *buf, ptrdiff_t len,
     if (p->outfd < 0)
       error ("Output file descriptor of %s is closed", SDATA (p->name));

  +#ifdef HAVE_GNUTLS
  +  /* The TLS connection hasn't been set up yet, so we can't write
  +     anything on the socket. */
  +  if (!NILP (p->gnutls_boot_parameters))
  +    return;
  +#endif

I don't think this is a good idea: you are silently returning here
without doing anything; the callers of send_process won't expect
that.  I think this should signal an error instead.

One final comment: I think this change will need addition of tests to
the test suite, to exercise the affected APIs, so we could make sure
we don't introduce any bugs in the existing functionalities.

Thanks.

======================================================================

> > Of course it does.  It's a major architecture change that we should
> > know about, and probably discuss before doing.
> 
> Ok.  :-)  The last time this was discussed, I mentioned threads and I
> think the response was basically "sure, if you can get it to work"...
> 
> What problems do you see with stopping/starting threads in Emacs (that
> do no Lisp-related stuff)?

We need to know which code can run on a separate thread, because some
things cannot be safely done from any thread but the main
(a.k.a. "Lisp") thread.  Running the Lisp interpreter is one of them,
but it's not the only one.  You cannot QUIT or signal an error or do
anything else that throws to top-level.  You cannot call malloc or
free, or any code that does.  You cannot modify any data structures
visible from Lisp, like buffers, Lisp strings, and the undo list.  You
cannot access or modify global variables or call any non-reentrant
Emacs functions.  And there are other no-no's, these are just a few
that popped in my mind within the first 5 sec.

So starting threads from Emacs application code is not something to
take easily, it should be justified and clearly marked in the sources.

However, it sounds like this is all much ado about nothing: I see no
references to any threads, old or new, in the changes you made on your
feature branch, so is there really anything to discuss here?  (I asked
the question which led to this sub-thread because you mentioned that
you "start a new thread that does getaddrinfo".)

> I'm assuming there are plenty of libraries that Emacs uses that
> already does this behind Emacs' back, and we don't seem to care...

First, we do care: the threads run by GTK, for example, caused us a
lot of grief at the time.  More importantly: those threads can hardly
run any Emacs code, can they?

> Anyway, this reminds me of something that I've been wondering about gdb
> output when running Emacs.  It often says something like this:
> 
> warning: Corrupted shared library list: 0x84582e0 != 0x5104c30
> warning: Corrupted shared library list: 0x2f172a0 != 0x21688a0
> warning: Corrupted shared library list: 0x79f1910 != 0x21688a0
> warning: Corrupted shared library list: 0x79f1910 != 0x21688a0
> warning: Corrupted shared library list: 0x7a07ae0 != 0x21688a0
> 
> Is that something to be worried about, or is it...  normal?

It's a real problem.  Crystal ball says that some of the system
libraries Emacs uses don't have debug info files to match them;
instead, you have debug info files from different versions of the
libraries.  Try "info sharedlibrary" at the GDB prompt, maybe it will
tell you which libraries are the offending ones.

If you cannot figure this out, it is best to ask on the GDB mailing
list.