From: storm@cua.dk (Kim F. Storm)
Cc: emacs-devel@gnu.org
Subject: Re: Non-blocking open-network-stream
Date: 22 Feb 2002 00:45:17 +0100 [thread overview]
Message-ID: <5xwux64cxe.fsf@kfs2.cua.dk> (raw)
In-Reply-To: <m2u1sa7819.fsf@xaital.online-marketwatch.com>
Helmut Eller <helmut@xaital.km4u.net> 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 <storm@cua.dk> http://www.cua.dk
_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel
next parent reply other threads:[~2002-02-21 23:45 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <m2u1sa7819.fsf@xaital.online-marketwatch.com>
2002-02-21 23:45 ` Kim F. Storm [this message]
2002-02-22 16:04 ` Non-blocking open-network-stream Stefan Monnier
2002-02-25 22:38 ` Kim F. Storm
2002-02-26 22:46 ` Helmut Eller
2002-02-27 11:59 ` Kim F. Storm
2002-02-28 4:08 ` Richard Stallman
2002-03-01 0:21 ` Kim F. Storm
2002-03-01 8:01 ` Juanma Barranquero
2002-03-01 10:50 ` Kim F. Storm
2002-03-01 17:10 ` Pavel Janík
2002-03-01 21:23 ` Richard Stallman
2002-03-07 0:08 ` New patch for server sockets and datagram (UDP) support Kim F. Storm
2002-03-07 10:56 ` Kim F. Storm
2002-03-07 11:39 ` Alex Schroeder
2002-03-07 12:39 ` Kim F. Storm
2002-03-07 14:51 ` Alex Schroeder
2002-03-08 21:06 ` Richard Stallman
2002-03-13 15:56 ` Kim F. Storm
2002-03-13 23:19 ` Final(?) " Kim F. Storm
2002-03-14 0:50 ` Al Petrofsky
2002-03-14 9:30 ` Kim F. Storm
2002-03-14 12:42 ` Richard Stallman
2002-03-14 13:35 ` Kim F. Storm
2002-03-17 22:02 ` I have installed the " Kim F. Storm
2002-03-07 15:18 ` New " Helmut Eller
2002-03-07 16:09 ` Kim F. Storm
2002-03-07 17:32 ` Helmut Eller
2002-03-07 23:58 ` Kim F. Storm
2002-03-08 7:38 ` Helmut Eller
2002-03-08 9:13 ` Kim F. Storm
2002-03-08 11:16 ` Helmut Eller
2002-03-08 16:36 ` Stefan Monnier
2002-03-08 20:57 ` Kim F. Storm
2002-03-08 21:03 ` Stefan Monnier
2002-03-08 21:07 ` Richard Stallman
2002-03-13 15:12 ` Kim F. Storm
2002-03-07 12:54 ` Mario Lang
2002-03-07 12:58 ` Kim F. Storm
2002-03-08 9:09 ` Richard Stallman
2002-03-08 9:35 ` Kim F. Storm
2002-03-08 11:04 ` Helmut Eller
2002-03-02 7:59 ` Non-blocking open-network-stream Helmut Eller
2002-03-03 0:12 ` Kim F. Storm
2002-03-03 10:46 ` Helmut Eller
2002-03-03 16:44 ` Mario Lang
2002-03-03 14:39 ` Richard Stallman
2002-02-27 17:49 Helmut Eller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5xwux64cxe.fsf@kfs2.cua.dk \
--to=storm@cua.dk \
--cc=emacs-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.