Re: Non-blocking open-network-stream

[Kim F. Storm]

Helmut Eller <address@hidden> 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 <address@hidden> http://www.cua.dk