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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> 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?

You don't see how taking the DNS resolution out of the "stop everything
Emacs is doing" bit makes Emacs stop everything it's doing for a shorter
while?  I think I must be misunderstanding you...

> 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?

Sure, but we were talking about the DNS resolution...

> 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.

Sure.  The optional part here is that the user says :nowait when they
don't want to wait, so I'm not sure I'm getting your objection here,
either.

> 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.

Ok, fixed.

>                    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.

It's currently obtained from `gnutls-negotiate' with a :return-keywords
parameter, but I think it would make sense to refactor that so that
there's a separate function that computes the parameter.  I'll fix that.

>   -@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.

Ok, fixed.

>   -(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.

Done.

>    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.

Yup.  :-)

>   +: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.

Will do.

> 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?

Oh yeah, that's true.  When I did that change I didn't know that a
status could be a list that returned more error info.  Fixed now.

>   +#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.

It's perfectly fine to say

(setq proc (make-network-process ... :nowait t :tls-parameters ...))
(process-send-string proc)

It's not an error, and the idle loop will try resending it until it
succeeds.  (Which will only happen after DNS resolution and TLS
negotiation has failed.)

If the user has requested a TLS socket, then it's nonsensical to try
sending data on it before it's completed the negotiation.

> 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.

Unfortunately, there are virtually no network tests in the suite.  At
least I couldn't find any.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no