From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#22493: 25.1.50; open-gnutls-stream doesn't respect :nowait, so the connections are synchronous Date: Tue, 02 Feb 2016 18:10:27 +0200 Message-ID: <83lh73ytxo.fsf@gnu.org> References: <87mvrnzpge.fsf@gnus.org> <878u37zndq.fsf@gnus.org> <83r3gzwhg8.fsf@gnu.org> <87fuxebrsy.fsf@gnus.org> <878u36fung.fsf@gnus.org> <8360y93fka.fsf@gnu.org> <87wpqpwd8p.fsf@gnus.org> <83d1sh14is.fsf@gnu.org> <87egcx13kc.fsf@gnus.org> <83bn801d06.fsf@gnu.org> <87bn7znazd.fsf@gnus.org> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: ger.gmane.org 1454429550 30072 80.91.229.3 (2 Feb 2016 16:12:30 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 2 Feb 2016 16:12:30 +0000 (UTC) Cc: 22493@debbugs.gnu.org To: Lars Ingebrigtsen Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Tue Feb 02 17:12:18 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1aQdYv-0004EZ-0T for geb-bug-gnu-emacs@m.gmane.org; Tue, 02 Feb 2016 17:12:17 +0100 Original-Received: from localhost ([::1]:57913 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQdYu-00013H-3x for geb-bug-gnu-emacs@m.gmane.org; Tue, 02 Feb 2016 11:12:16 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:52153) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQdYj-0000zh-VO for bug-gnu-emacs@gnu.org; Tue, 02 Feb 2016 11:12:12 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQdYg-00051O-3z for bug-gnu-emacs@gnu.org; Tue, 02 Feb 2016 11:12:05 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:48399) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQdYg-00051K-0z for bug-gnu-emacs@gnu.org; Tue, 02 Feb 2016 11:12:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84) (envelope-from ) id 1aQdYf-0003CD-Pb for bug-gnu-emacs@gnu.org; Tue, 02 Feb 2016 11:12:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 02 Feb 2016 16:12:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 22493 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 22493-submit@debbugs.gnu.org id=B22493.145442946612213 (code B ref 22493); Tue, 02 Feb 2016 16:12:01 +0000 Original-Received: (at 22493) by debbugs.gnu.org; 2 Feb 2016 16:11:06 +0000 Original-Received: from localhost ([127.0.0.1]:56988 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84) (envelope-from ) id 1aQdXj-0003Ar-5g for submit@debbugs.gnu.org; Tue, 02 Feb 2016 11:11:06 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:60945) by debbugs.gnu.org with esmtp (Exim 4.84) (envelope-from ) id 1aQdXe-0003AL-9V for 22493@debbugs.gnu.org; Tue, 02 Feb 2016 11:11:01 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQdXU-0004rX-9Q for 22493@debbugs.gnu.org; Tue, 02 Feb 2016 11:10:53 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:54398) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQdXU-0004rM-5T; Tue, 02 Feb 2016 11:10:48 -0500 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:1989 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1aQdXT-0002pK-Da; Tue, 02 Feb 2016 11:10:47 -0500 In-reply-to: <87bn7znazd.fsf@gnus.org> (message from Lars Ingebrigtsen on Tue, 02 Feb 2016 02:43:02 +0100) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:112273 Archived-At: > From: Lars Ingebrigtsen > Cc: 22493@debbugs.gnu.org > 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.