From mboxrd@z Thu Jan 1 00:00:00 1970 Path: quimby.gnus.org!not-for-mail From: storm@cua.dk (Kim F. Storm) Newsgroups: gmane.emacs.devel Subject: Re: Non-blocking open-network-stream Date: 22 Feb 2002 00:45:17 +0100 Message-ID: <5xwux64cxe.fsf@kfs2.cua.dk> References: NNTP-Posting-Host: quimby2.netfonds.no Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: quimby2.netfonds.no 1014335224 3845 195.204.10.66 (21 Feb 2002 23:47:04 GMT) X-Complaints-To: usenet@quimby2.netfonds.no NNTP-Posting-Date: 21 Feb 2002 23:47:04 GMT Cc: emacs-devel@gnu.org Original-Received: from fencepost.gnu.org ([199.232.76.164]) by quimby2.netfonds.no with esmtp (Exim 3.12 #1 (Debian)) id 16e2vX-0000zv-00 for ; Fri, 22 Feb 2002 00:47:03 +0100 Original-Received: from localhost ([127.0.0.1] helo=fencepost.gnu.org) by fencepost.gnu.org with esmtp (Exim 3.33 #1 (Debian)) id 16e2tq-0005p9-00; Thu, 21 Feb 2002 18:45:18 -0500 Original-Received: from mail.filanet.dk ([195.215.206.179]) by fencepost.gnu.org with smtp (Exim 3.33 #1 (Debian)) id 16e2sl-0005Xd-00 for ; Thu, 21 Feb 2002 18:44:11 -0500 Original-Received: from kfs2.cua.dk.cua.dk (unknown [10.1.82.3]) by mail.filanet.dk (Postfix) with SMTP id 584137C035; Thu, 21 Feb 2002 23:44:09 +0000 (GMT) Original-To: Helmut Eller In-Reply-To: Original-Lines: 190 User-Agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.2.50 Errors-To: emacs-devel-admin@gnu.org X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.0.5 Precedence: bulk List-Help: List-Post: List-Subscribe: , List-Id: Emacs development discussions. List-Unsubscribe: , List-Archive: Xref: quimby.gnus.org gmane.emacs.devel:1404 X-Report-Spam: http://spam.gmane.org/gmane.emacs.devel:1404 Helmut Eller writes: > Hi, > > I read your post about non-blocking open-network-stream in the web > archive of the emacs-devel list and would like to make some comments. > I write to you directly because I'm not subscribed and I have the > impression the list is "For Developers Only". > Thanks a lot for your feedback. I've copied my response to the list. > I tried the code a bit and I noticed that the sentinel is sometimes > not invoked with "run" when the connections completes successfully but > is closed shortly afterwards by the peer. The sentinel was sometimes > invoked sometimes not. The scenario was very simple: a server > accepted the connection and wrote "hello" to the socket and closed the > connection. Emacs executed this: > > (open-network-stream "nb" "x.x" "localhost" 34567 > nil > (lambda (s msg) > (with-current-buffer (process-buffer s) > (insert msg "\n") > (when (equal "failed" msg) > (message "deleting %S" s) > (delete-process s)))) > t) > This seems to be related to the problem you describe below: reading the process output before checking for the completion of the connect. I'll look into it. > You wrote: > > [...] > > The initial process-state is `connecting' which changes to `open' > > or `failed' depending on whether the connect succeeded or failed. > > Notice that the sentinel string for `open' is "run". > > You could modify status_message to return something more meaningful. > Preferably the symbol 'open. > Yes, I considered doing that. However, I didn't know whether existing code may depend on the current behaviour, and although I couldn't find any in CVS, I decided against changing it. > [...] > > + /* Number of bits set in connect_wait_mask. */ > > + int num_pending_connects; > > + > > Any reason to make this non-static? No. I just forgot it. > > [...] > > ! non_blocking = (NILP (non_blocking) ? Qnil : Qt); > > ! #ifdef NON_BLOCKING_CONNECT > > ! is_non_blocking = !NILP (non_blocking); > > ! #else > > ! is_non_blocking = 0; > > ! #endif > > Wouldn't non_blocking_p be a more lispy name? :-) Yes - but is_non_blocking is not a lisp object :-) > > [...] > > --- 1948,1971 ---- > > turn_on_atimers (1); > > > > if (ret == 0 || xerrno == EISCONN) > > ! { > > ! is_non_blocking = 0; > > ! /* The unwind-protect will be discarded afterwards. > > ! Likewise for immediate_quit. */ > > ! break; > > ! } > > Why do you set is_non_blocking to 0? I can see no later use. For > documentation? It is defensive programming -- in case it ever becomes necessary to test on it later in the code, it contains the proper value. > > > ! > > ! #ifdef NON_BLOCKING_CONNECT > > ! #ifdef EINPROGRESS > > ! if (is_non_blocking && xerrno == EINPROGRESS) > > ! break; > > ! #else > > ! #ifdef EWOULDBLOCK > > ! if (is_non_blocking && xerrno == EWOULDBLOCK) > > break; > > + #endif > > + #endif > > What does it mean when connect returns EWOULDBLOCK? My man page > doesn't mention it. > Neither does mine -- but I saw some references on the Web which mentions this as one of the possible error codes from a non-blocking connect. So I included the test in case EINPROGRESS is not defined. > [...] > > --- 2176,2202 ---- > [...] > > ! if (!NILP (non_blocking)) > > ! { > > ! XPROCESS (proc)->status = Qconnecting; > > ! if (!FD_ISSET (inch, &connect_wait_mask)) > > ! { > > ! FD_SET (inch, &connect_wait_mask); > > ! num_pending_connects++; > > ! } > > ! } > > Why is if(!FD_ISET...) necessary? Because num_pending_connects would be updated incorrectly if it is already set. Defensive programming... > > [...] > > + #ifdef NON_BLOCKING_CONNECT > > + if (check_connect && FD_ISSET (channel, &Connecting)) > > + { > > Isn't it possible that channel becomes readable and writable at the > same time? If yes, wouldn't that mean that read_process_output (and > hence the filter) was already called before we get here? It seems you are right. Maybe the easiest fix would be for read_process_output to just return 0 if the process state is Qconnecting. I'll look into that. > > > + struct Lisp_Process *p; > > + struct sockaddr pname; > > + socklen_t pnamelen = sizeof(pname); > > + > > + FD_CLR (channel, &connect_wait_mask); > > + if (--num_pending_connects < 0) > > + abort (); > > + > > + proc = chan_process[channel]; > > + if (NILP (proc)) > > + continue; > > Is it safe to decrement num_pending_connects even if proc is nil? > > [...] Yes, because we only get here if channel is in the Connecting mask which is a subset of connect_wait_mask. Whether there really is a corresponding process doesn't matter. (there should be one, but I'm playing safe here -- cleaning up the connect_wait_mask in any case). > > + > > + p = XPROCESS (proc); > > + XSETINT (p->tick, ++process_tick); > > + > > + /* If connection failed, getpeername fails */ > > + if (getpeername(channel, &pname, &pnamelen) < 0) > > + { > > + /* Preserve status of processes already terminated. */ > > + p->status = Qfailed; > > + deactivate_process (proc); > > + } > > + else > > It would be nice if the error message (obtained with getsockopt > (channel, SOL_SCOKET, SO_ERROR ...)) would be passed to the sentinel. > That would be reasonable. I'll look into that. > A minor note: open-network-stream contains a large chunk of duplicated > code (starting from "/* Kernel bugs (on Ultrix..." to > "report_file_error ("connection failed...))). This are about 70 lines; > should they be in a separate function? Maybe, or the #ifdefs could be rearranged to avoid the duplication. I'll take a look. -- Kim F. Storm http://www.cua.dk _______________________________________________ Emacs-devel mailing list Emacs-devel@gnu.org http://mail.gnu.org/mailman/listinfo/emacs-devel