Asynchronous DNS

Asynchronous DNS

[Lars Magne Ingebrigtsen]

I talked about this a few years ago, but I didn't get very far, so I
thought I'd reopen the subject and actually implement something this
time.

The problem is a general one: Everything about network communication in
Emacs is asynchronous, except name resolution.  In many cases, this
isn't really that much of a problem, because whatever Emacs is doing has
to wait for the entire network process to finish (say, copying a file
over tramp), so that one part is synchronous isn't very noticeable.

The one place it's really noticeable is in eww.  You go to a page, and
then Emacs will freeze (sometimes for many seconds), and then Emacs will
unfreeze and start inserting images into the page.  The freeze bit is
when it's doing name resolution in `make-network-process' by calling
getaddrinfo or gethostbyname.

I see four ways of implementing an async name resolution function:

1) Add a new `resolve-name' function that would fork Emacs, call
getaddrinfo/gethostbyname, and do a callback once we get a reply.  The
problem is that a forked Emacs is a very strange thing, and is not
something we otherwise do.  I looked at this for a day last time, and I
couldn't get it to work, really.  Perhaps somebody else could, though.

2) Use one of the newer async C resolver libraries.  This would not be
available on all platforms, and would be Yet Another Dependency which
I'm sure everybody would rather that we avoid.

3) Create a tiny helper program in lib-src that would read names from
stdin and output resolves on stdout.  It would use
getaddrinfo/gethostbyname, and could very well be multithreaded, so that
it could do name resolution on parallel.

4) Pick the data out of res_init to find the DNS servers, and then just
use dns.el for name resolution.  This would be trivial to implement, but
may not respect the name resolutions settings on some operating systems.

In either case, I envision anything that wants async name resolution to
basically transform from

(make-network-process :host "foo.bar" ...)

to

(resolve-name "foo.bar"
  (lambda (status ip)
    (make-network-process :host "foo.bar" :ip ip ...)))

or something along those lines.  That is -- pass in a preresolved IP
address to `make-network-process'.

(Or list of addresses in the case of multihomed hosts, of course.
Details, details.)

So what do you think?  I'm up for implementing 3) or 4), but probably
not 1) or 2), but I think 1) would be the cleanest way to implement
this, if possible.

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

Re: Asynchronous DNS

[Elias Mårtenson]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

On 23 January 2016 at 21:50, Lars Magne Ingebrigtsen <larsi@gnus.org> wrote:


> I see four ways of implementing an async name resolution function:
>
> 1) Add a new `resolve-name' function that would fork Emacs, call
> getaddrinfo/gethostbyname, and do a callback once we get a reply.  The
> problem is that a forked Emacs is a very strange thing, and is not
> something we otherwise do.  I looked at this for a day last time, and I
> couldn't get it to work, really.  Perhaps somebody else could, though.
>
> 2) Use one of the newer async C resolver libraries.  This would not be
> available on all platforms, and would be Yet Another Dependency which
> I'm sure everybody would rather that we avoid.
>
> 3) Create a tiny helper program in lib-src that would read names from
> stdin and output resolves on stdout.  It would use
> getaddrinfo/gethostbyname, and could very well be multithreaded, so that
> it could do name resolution on parallel.
>
> 4) Pick the data out of res_init to find the DNS servers, and then just
> use dns.el for name resolution.  This would be trivial to implement, but
> may not respect the name resolutions settings on some operating systems.
>

This is something I have been wondering for a while, and I think that this
may be
as good a time as any to ask the question.

Are there any technical reasons why the name resolution (or whatever it is
that
needs to be asynchronous) could not be run in a thread in the Emacs process
itself? As long as the thread is not running any Lisp code, it should work,
correct?

I'm asking this because I've had some ideas on modules that I would write
once
the loadable module support in Emacs is complete, and in some cases I want
to be able to do stuff in threads.

Regards,
Elias

[-- Attachment #2: Type: text/html, Size: 2286 bytes --]

Re: Asynchronous DNS

[Paul Eggert]

Lars Magne Ingebrigtsen wrote:
> 2) Use one of the newer async C resolver libraries.  This would not be
> available on all platforms, and would be Yet Another Dependency which
> I'm sure everybody would rather that we avoid.

Nevertheless, this sounds like the most promising approach. Why reinvent the 
wheel? Platforms lacking the library will be no worse off than they are now.

Do you consider GNU adns to be one of the "newer" libraries? If not, what 
problems do you see with GNU adns?

Re: Asynchronous DNS

[Stefan Monnier]

> (resolve-name "foo.bar"
>   (lambda (status ip)
>     (make-network-process :host "foo.bar" :ip ip ...)))

If possible, I'd rather the async-dns-resolution be done directly as
part of make-network-process.


        Stefan

Re: Asynchronous DNS

[Lars Magne Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Lars Magne Ingebrigtsen wrote:
>> 2) Use one of the newer async C resolver libraries.  This would not be
>> available on all platforms, and would be Yet Another Dependency which
>> I'm sure everybody would rather that we avoid.
>
> Nevertheless, this sounds like the most promising approach. Why
> reinvent the wheel? Platforms lacking the library will be no worse off
> than they are now.
>
> Do you consider GNU adns to be one of the "newer" libraries? If not,
> what problems do you see with GNU adns?

I haven't looked closely at any of the asynchronous C libraries, because
I assumed that it was off the table.  But if it's fine to add another
(optional) C library to the Emacs build, then I'm up for investigating
further and taking a stab at implementing it.

The most well-distributed C-level libraries for async DNS seem to be
adns, c-ares and firedns, at least on Linux systems.  adns has no
documentation, but has a very well-commented C header file, and looks,
at first blush, to provide a sane interface.

firedns looks pretty abandoned.  c-ares seems sane and has
documentation:

https://github.com/c-ares/c-ares

udns seems even better:  http://www.corpit.ru/mjt/udns.html
Unfortunately, it doesn't look like it's available on Debian, for
instance, which limits its practicality.

I have no experience with either of these libraries in practice...
Anybody have any input here?

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

Re: Asynchronous DNS

[Lars Magne Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> (resolve-name "foo.bar"
>>   (lambda (status ip)
>>     (make-network-process :host "foo.bar" :ip ip ...)))
>
> If possible, I'd rather the async-dns-resolution be done directly as
> part of make-network-process.

If we go for a C-level solution here, then that would definitely be
preferable.  `make-network-process' would then take an :async parameter
and return as soon as it's started name resolution, returning a process
object in a new "resolving" state.  The connection would be completed
from the DNS callback, or from the event loop, possibly.

Error handling would be somewhat different in :async, of course.

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

Re: Asynchronous DNS

[Lars Magne Ingebrigtsen]

Oh, and there's a getaddrinfo_a now in glibc?  I didn't know that...
Then what are all these other libraries doing?

I'll read the manpage.

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

Re: Asynchronous DNS

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> Oh, and there's a getaddrinfo_a now in glibc?  I didn't know that...
> Then what are all these other libraries doing?
>
> I'll read the manpage.

getaddrinfo_a looks quite straightforward.  There are two modes: We can
either get a signal back once we get a response, or we can poll to see
whether there's any response.  I guess polling would fit quite well with
the Emacs event loop?  That would be somewhere in
wait_reading_process_output, I guess.

It would then call gai_error on all sockets that are in the "resolving"
state to determine whether there's a response (positive, negative,
timeout) or not, and either continue the connection process, or mark the
process as dead.

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

Re: Asynchronous DNS

[Lars Magne Ingebrigtsen]

I've been reading make-network-process, and it's, er, complicated code.
Three levels of nesting #ifdefs.  But I think it can be separated into
two functions without breaking too much -- one that does (or starts
doing) name resolution, and one that calls connect (and the stuff after
that).

Doing asynch DNS can be triggered by the existing :nowait parameter
without breaking any existing callers, I think.

So: If called without :nowait, the first half of the function will call
the second one directly.  If called with :nowait, the second half of the
function will be called from within the bowels of
wait_reading_process_output somewhere.

Sound like a plan?  If so I'll start implementing on...  I think
Wednesday.  I may need some help on the autoconf stuff for
getaddrinfo_a, though.  :-)

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

Re: Asynchronous DNS

[Paul Eggert]

Lars Magne Ingebrigtsen wrote:
> getaddrinfo_a looks quite straightforward.

Yes, and it has the advantage of not needing another library. It's less 
portable, but people interested in non-GNU ports can add them at their leisure.

Re: Asynchronous DNS

[Lars Magne Ingebrigtsen]

Elias Mårtenson <lokedhs@gmail.com> writes:

> Are there any technical reasons why the name resolution (or whatever
> it is that needs to be asynchronous) could not be run in a thread in
> the Emacs process itself? As long as the thread is not running any
> Lisp code, it should work, correct?

There's no problem doing non-Lisp things in a separate thread.  But
doing anything that calls Lisp stuff from the non-main thread is, if I
understand things correctly, highly iffy.

Which makes things rather awkward.

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

Re: Asynchronous DNS

[Stefan Monnier]

> But I think it can be separated into two functions without breaking

Sounds good.

> Doing asynch DNS can be triggered by the existing :nowait parameter
> without breaking any existing callers, I think.

That's also my impression.


        Stefan

Re: Asynchronous DNS

[Elias Mårtenson]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On 25 January 2016 at 05:17, Lars Magne Ingebrigtsen <larsi@gnus.org> wrote:


> There's no problem doing non-Lisp things in a separate thread.  But
> doing anything that calls Lisp stuff from the non-main thread is, if I
> understand things correctly, highly iffy.
>
> Which makes things rather awkward.


Thank you for the information.

Don't you think it would make sense to add some kind of standardised
functionality to Emacs that allows native code running in a thread to
asynchronously call a callback function in the Lisp code? The function
would be added to the event queue to be run in the Emacs main thread at
some point in the future.

In essence, I'm suggesting something similar to
SwingUtilities.invokeLater() from Java.

Regards,
Elias

[-- Attachment #2: Type: text/html, Size: 1175 bytes --]

Re: Asynchronous DNS

[Eli Zaretskii]

> Date: Mon, 25 Jan 2016 16:58:03 +0800
> From: Elias Mårtenson <lokedhs@gmail.com>
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> Don't you think it would make sense to add some kind of standardised
> functionality to Emacs that allows native code running in a thread to
> asynchronously call a callback function in the Lisp code? The function would be
> added to the event queue to be run in the Emacs main thread at some point in
> the future.

The challenge with this scheme is to come up with the way to add an
event to the Emacs event queue from another thread.  Once the event is
in the queue, all the rest is simple.  However, the event queue is not
currently designed to accept events from other threads.

Maybe you should take a look at the concurrency branch.

Re: Asynchronous DNS

[Elias Mårtenson]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

On 25 Jan 2016 23:51, "Eli Zaretskii" <eliz@gnu.org> wrote:
>
> The challenge with this scheme is to come up with the way to add an
> event to the Emacs event queue from another thread.  Once the event is
> in the queue, all the rest is simple.  However, the event queue is not
> currently designed to accept events from other threads.
>
> Maybe you should take a look at the concurrency branch.

I did something similar in a different project, and I solved that by
creating a pipe so that the main thread's event handler (which was blocking
on a select() call) could be notified that the worker thread wanted to send
a message to the main loop.

Isn't Emacs doing something similar, and if so, couldn't the same mechanism
be used?

Regards,
Elias

[-- Attachment #2: Type: text/html, Size: 958 bytes --]

Re: Asynchronous DNS

[Eli Zaretskii]

> Date: Tue, 26 Jan 2016 01:15:56 +0800
> From: Elias Mårtenson <lokedhs@gmail.com>
> Cc: emacs-devel <emacs-devel@gnu.org>, Lars Magne Ingebrigtsen <larsi@gnus.org>
> > The challenge with this scheme is to come up with the way to add an
> > event to the Emacs event queue from another thread. Once the event is
> > in the queue, all the rest is simple. However, the event queue is not
> > currently designed to accept events from other threads.
> >
> > Maybe you should take a look at the concurrency branch.
> 
> I did something similar in a different project, and I solved that by creating a
> pipe so that the main thread's event handler (which was blocking on a select()
> call) could be notified that the worker thread wanted to send a message to the
> main loop. 

I sincerely hope we will be able to come up with something more
elegant.

> Isn't Emacs doing something similar

Doing where?  In the concurrency branch?  No, of course not.  Frankly,
I'm not even sure that branch is up to the job, it's been a long time
since I looked at the code (and the name "concurrency" could deceive
in this case).

Re: Asynchronous DNS

[John Wiegley]

>>>>> Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> 1) Add a new `resolve-name' function that would fork Emacs, call
> getaddrinfo/gethostbyname, and do a callback once we get a reply. The
> problem is that a forked Emacs is a very strange thing, and is not something
> we otherwise do. I looked at this for a day last time, and I couldn't get it
> to work, really. Perhaps somebody else could, though.

BTW- this exactly what the 'async' library now in GNU ELPA was written to do.
You could use it at least as a first approximation.

Due to expense, I'd recommend collecting as many names to resolve at a time as
you can, and then doing them asynchronously "in batch"; I can think of a few
web pages that would suddenly result in 20-30 Emacsen being forked.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Asynchronous DNS

[Florian Weimer]

On 01/23/2016 02:50 PM, Lars Magne Ingebrigtsen wrote:
> 2) Use one of the newer async C resolver libraries.  This would not be
> available on all platforms, and would be Yet Another Dependency which
> I'm sure everybody would rather that we avoid.

The internal NSS (Name Service Switch) API is not asynchronous, so a
fully backwards-compatible asynchronous name resolution library simply
cannot exist.  The so-called asynchronous getaddrinfo variant just uses
threads under the covers, too.

You could use threads or a co-process, though.  It's not too hard to
implement as long as you don't have to care about head-of-line blocking.
 You could even a subshell invoking “getent ahosts” and parse the result.

Florian

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Async DNS has now been implemented in the feature/async-dns branch, and
I'm running that Emacs right now.  And it works!  I mean, eww no longer
gets those awkward pauses when resolving DNS names of image assets.
Whee!  Or placebo!  Whee!

There's quite a bit of cleanup remaining, though, and I'm probably
leaking memory ... somewhere...  And I'm not really happy with the way
I'm doing the polling, which required another process array in addition
to the chan_process table.  Surely a better way must exist, but the
process isn't entered into the chan_process table until the socket has
been created, and we can't create the socket before we've done the name
resolution, because we don't know whether it's going to be an IPv4
address or an IPv6 address.

If anybody else wants to start fiddling with the stuff, be my guest.
:-)  But I'm going to continue tinkering with it on the branch, and will
probably merge with the trunk in a few days.  It would be nice if people
on non-Linux systems could check whether it builds at all on those
systems before I do the merge...

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

Re: Asynchronous DNS

[Alex Dunn]

The build is failing for me on Mac OS 10.11.3:
https://gist.github.com/dunn/ffca0bcfbf97d57d9c7d#file-03-make-L426-L451

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Async DNS has now been implemented in the feature/async-dns branch, and
> I'm running that Emacs right now.  And it works!  I mean, eww no longer
> gets those awkward pauses when resolving DNS names of image assets.
> Whee!  Or placebo!  Whee!
>
> There's quite a bit of cleanup remaining, though, and I'm probably
> leaking memory ... somewhere...  And I'm not really happy with the way
> I'm doing the polling, which required another process array in addition
> to the chan_process table.  Surely a better way must exist, but the
> process isn't entered into the chan_process table until the socket has
> been created, and we can't create the socket before we've done the name
> resolution, because we don't know whether it's going to be an IPv4
> address or an IPv6 address.
>
> If anybody else wants to start fiddling with the stuff, be my guest.
> :-)  But I'm going to continue tinkering with it on the branch, and will
> probably merge with the trunk in a few days.  It would be nice if people
> on non-Linux systems could check whether it builds at all on those
> systems before I do the merge...
>
> -- 
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alex Dunn <dunn.alex@gmail.com> writes:

> The build is failing for me on Mac OS 10.11.3:
> https://gist.github.com/dunn/ffca0bcfbf97d57d9c7d#file-03-make-L426-L451

Thanks; should be fixed now.

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

Re: Asynchronous DNS

[Alex Dunn]

Thanks for the quick reply!  It gets further now, but runs into a
linking error (library not found for -lanl):
https://gist.github.com/dunn/8df8eb0e12fa2f870e29#file-03-make-L441

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Dunn <dunn.alex@gmail.com> writes:
>
>> The build is failing for me on Mac OS 10.11.3:
>> https://gist.github.com/dunn/ffca0bcfbf97d57d9c7d#file-03-make-L426-L451
>
> Thanks; should be fixed now.
>
> -- 
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alex Dunn <dunn.alex@gmail.com> writes:

> Thanks for the quick reply!  It gets further now, but runs into a
> linking error (library not found for -lanl):
> https://gist.github.com/dunn/8df8eb0e12fa2f870e29#file-03-make-L441

Hm...  Even after all these years, I still have no idea how auto...make?
tools?  Work.  I cargo culted and ended up with this:

GETADDRINFO_A_LIBS="-lanl"
AC_CHECK_LIB(anl, getaddrinfo_a, HAVE_GETADDRINFO_A=yes, HAVE_GETADDRINFO_A=no,
        [$GETADDRINFO_A_LIBS])
AC_SUBST(GETADDRINFO_A_LIBS)

OLD_LIBS=$LIBS
LIBS="-lanl $LIBS"
AC_CHECK_FUNCS(getaddrinfo_a)
LIBS=$OLD_LIBS

Which doesn't do the right thing, apparently.  Can somebody who knows
what this language is fix the magical incantation?

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

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sat, 30 Jan 2016 01:11:40 +0100
> 
> Async DNS has now been implemented in the feature/async-dns branch, and
> I'm running that Emacs right now.  And it works!  I mean, eww no longer
> gets those awkward pauses when resolving DNS names of image assets.
> Whee!  Or placebo!  Whee!
> 
> There's quite a bit of cleanup remaining, though, and I'm probably
> leaking memory ... somewhere...  And I'm not really happy with the way
> I'm doing the polling, which required another process array in addition
> to the chan_process table.  Surely a better way must exist, but the
> process isn't entered into the chan_process table until the socket has
> been created, and we can't create the socket before we've done the name
> resolution, because we don't know whether it's going to be an IPv4
> address or an IPv6 address.
> 
> If anybody else wants to start fiddling with the stuff, be my guest.
> :-)  But I'm going to continue tinkering with it on the branch, and will
> probably merge with the trunk in a few days.  It would be nice if people
> on non-Linux systems could check whether it builds at all on those
> systems before I do the merge...

Please also be sure to update the documentation for the new features.

Thanks.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Please also be sure to update the documentation for the new features.

I don't think there's anything to document.  A mention in NEWS may be
appropriate, though, but even there I'm not sure.

:nowait just becomes a bit less wait-ey.

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

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Sat, 30 Jan 2016 08:46:32 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Please also be sure to update the documentation for the new features.
> 
> I don't think there's anything to document.

Is the below still correct and accurate, including the values of the
argument with which the sentinel will be called?

    :nowait BOOL
          If BOOL is non-`nil' for a stream connection, return without
          waiting for the connection to complete.  When the connection
          succeeds or fails, Emacs will call the sentinel function,
          with a second argument matching `"open"' (if successful) or
          `"failed"'.  The default is to block, so that
          `make-network-process' does not return until the connection
          has succeeded or failed.

What happens if the async DNS fails?

Also, do we want to allow for testing this separately in the likes of

  (featurep 'make-network-process '(:nowait t))

> A mention in NEWS may be appropriate, though, but even there I'm not
> sure.

I am sure it's appropriate.  Please do, and thanks.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Is the below still correct and accurate, including the values of the
> argument with which the sentinel will be called?
>
>     :nowait BOOL
>           If BOOL is non-`nil' for a stream connection, return without
>           waiting for the connection to complete.  When the connection
>           succeeds or fails, Emacs will call the sentinel function,
>           with a second argument matching `"open"' (if successful) or
>           `"failed"'.  The default is to block, so that
>           `make-network-process' does not return until the connection
>           has succeeded or failed.

Yes.  If not, it's a bug.

> What happens if the async DNS fails?

It marks the process as "failed".  It should probably say what kind of
failure it is, but I haven't investigated that part of the sentinel
system yet...

> Also, do we want to allow for testing this separately in the likes of
>
>   (featurep 'make-network-process '(:nowait t))

I don't really think so...  The level of asynchronicity (that should be
a word) should make no programmatic difference.

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

Re: Asynchronous DNS

[Ken Raeburn]

> On Jan 30, 2016, at 02:46, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>> Please also be sure to update the documentation for the new features.
> 
> I don't think there's anything to document.  A mention in NEWS may be
> appropriate, though, but even there I'm not sure.
> 
> :nowait just becomes a bit less wait-ey.

If I understand correctly, passing a name that can’t be resolved, in “nowait” mode, used to raise an error but now will return a process object that will report an error?  If async resolving isn’t enabled, which way will Emacs report a failure to resolve?

Ken

Re: Asynchronous DNS

[Andy Moreton]

On Sat 30 Jan 2016, Lars Ingebrigtsen wrote:

> Async DNS has now been implemented in the feature/async-dns branch, and
> I'm running that Emacs right now.  And it works!  I mean, eww no longer
> gets those awkward pauses when resolving DNS names of image assets.
> Whee!  Or placebo!  Whee!
>
> There's quite a bit of cleanup remaining, though, and I'm probably
> leaking memory ... somewhere...  And I'm not really happy with the way
> I'm doing the polling, which required another process array in addition
> to the chan_process table.  Surely a better way must exist, but the
> process isn't entered into the chan_process table until the socket has
> been created, and we can't create the socket before we've done the name
> resolution, because we don't know whether it's going to be an IPv4
> address or an IPv6 address.
>
> If anybody else wants to start fiddling with the stuff, be my guest.
> :-)  But I'm going to continue tinkering with it on the branch, and will
> probably merge with the trunk in a few days.  It would be nice if people
> on non-Linux systems could check whether it builds at all on those
> systems before I do the merge...

This doesn't build on mingw64:

../../src/process.c: In function 'Fmake_network_process':
../../src/process.c:3589:21: error: 'AF_LOCAL' undeclared (first use in this function)
       if (family != AF_LOCAL)
                     ^
../../src/process.c:3589:21: note: each undeclared identifier is reported only once for each function it appears in
../../src/process.c:3755:19: warning: implicit declaration of function 'Ncons' [-Wimplicit-function-declaration]
    ip_addresses = Ncons (make_number (host_info_ptr->h_addr,
                   ^
../../src/process.c:3756:34: error: macro "make_number" passed 2 arguments, but takes just 1
           host_info_ptr->h_length),
                                  ^
../../src/process.c:3755:17: error: incompatible types when assigning to type 'Lisp_Object {aka struct <anonymous>}' from type 'int'
    ip_addresses = Ncons (make_number (host_info_ptr->h_addr,
                 ^
../../src/process.c:3767:17: error: incompatible types when assigning to type 'Lisp_Object {aka struct <anonymous>}' from type 'int'
    ip_addresses = Ncons (make_number (numeric_addr), Qnil);
                 ^
../../src/process.c: In function 'send_process':
../../src/process.c:5831:7: error: used struct type value where scalar is required
   if (p->gnutls_async_parameters)
       ^
Makefile:372: recipe for target 'process.o' failed

The AF_LOCAL check looks like it should be guarded with
HAVE_LOCAL_SOCKETS.

The make_number() errors may be from configured with checking enabled.
"config.status --config" for this build shows:
'--prefix=c:/emacs' '--build=x86_64-w64-mingw32' '--without-dbus'
'--enable-checking' '--enable-check-lisp-object-type' '--with-gif'
'--with-jpeg' '--with-png' '--with-tiff' '--with-xpm' '--with-gnutls'
'--with-rsvg' '--with-xml2' '--without-imagemagick' '--with-modules'
'CFLAGS= -g3 -O0' 'build_alias=x86_64-w64-mingw32'

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> This doesn't build on mingw64:
>
> ../../src/process.c: In function 'Fmake_network_process':
> ../../src/process.c:3589:21: error: 'AF_LOCAL' undeclared (first use in this function)
>        if (family != AF_LOCAL)
>                      ^

This should now be fixed...

> ../../src/process.c:3589:21: note: each undeclared identifier is reported only once for each function it appears in
> ../../src/process.c:3755:19: warning: implicit declaration of function 'Ncons' [-Wimplicit-function-declaration]
>     ip_addresses = Ncons (make_number (host_info_ptr->h_addr,
>                    ^

Oops.  I obviously hadn't compiled the non-glibc code path at all.  I've
now fixed the non-HAVE_GETADDRINFO code paths, I think.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Both GnuTLS and normal sockets now use asynchronous DNS resolution, if
available.

I've reworked how TLS connections are set up, too as a result, and I
think the new version is a bit more logical.

Previously you called make-network-socket, and then called gnutls-boot
on that socket to "make it into" a TLS socket, which is a kinda low
level of doing that stuff.  You can still do that, but you can now also
pass in the GnuTLS boot parameters with the new :tls-parameters keyword,
and then make-network-socket will do everything for you.

As far as I can tell, the code works both with and without getaddrinfo,
getaddrinfo_a, TLS and nowait.  That's quite a few combinations.  :-)

I've also skimmed the getaddrinfo_a source code, and it's kinda
trivial.  Perhaps it would be a good idea to just replace the
getaddrinfo_a stuff with our own little async DNS library.

Basically all it does is start a new thread that does getaddrinfo, and
then stashes the response somewhere.  The library supports doing more
than that (async callbacks etc), but we don't need all that stuff.  So
perhaps it makes more sense just to implement this ourselves, and then
it would also work on non-glibc systems...

(We'd need to support gethostbyname, too, of course.)

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Ken Raeburn <raeburn@raeburn.org> writes:

> If I understand correctly, passing a name that can’t be resolved, in
> “nowait” mode, used to raise an error but now will return a process
> object that will report an error?

Yes, that's true.  So there is a user-visible difference here.  But the
documentation of make-network-process doesn't specify when that erroring
is supposed to happen on :nowait, so anybody relying on that behaviour
are doing something wrong...

> If async resolving isn’t enabled, which way will Emacs report a
> failure to resolve?

The same way as previously.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  Even after all these years, I still have no idea how auto...make?
> tools?  Work.  I cargo culted and ended up with this:

I joined another cult and came up with this:

AC_CHECK_LIB(anl, getaddrinfo_a, HAVE_GETADDRINFO_A=yes)
if test "${HAVE_GETADDRINFO_A}" = "yes"; then
  AC_DEFINE(HAVE_GETADDRINFO_A, 1,
[Define to 1 if you have getaddrinfo_a for asynchronous DNS resolution.])
  GETADDRINFO_A_LIBS="-lanl"
  AC_SUBST(GETADDRINFO_A_LIBS)
fi

Which seems to work...  I think...

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

Re: Asynchronous DNS

[Andy Moreton]

On Mon 01 Feb 2016, Lars Ingebrigtsen wrote:

> Andy Moreton <andrewjmoreton@gmail.com> writes:
>
>> This doesn't build on mingw64:
>>
>> ../../src/process.c: In function 'Fmake_network_process':
>> ../../src/process.c:3589:21: error: 'AF_LOCAL' undeclared (first use in this function)
>>        if (family != AF_LOCAL)
>>                      ^
>
> This should now be fixed...
>
>> ../../src/process.c:3589:21: note: each undeclared identifier is reported only once for each function it appears in
>> ../../src/process.c:3755:19: warning: implicit declaration of function 'Ncons' [-Wimplicit-function-declaration]
>>     ip_addresses = Ncons (make_number (host_info_ptr->h_addr,
>>                    ^
>
> Oops.  I obviously hadn't compiled the non-glibc code path at all.  I've
> now fixed the non-HAVE_GETADDRINFO code paths, I think.

Thanks. this gets further, but still has issues.
On a mingw64 build (configured with --enable-check-lisp-object-type):

../../src/process.c: In function ‘send_process’:
../../src/process.c:5907:7: error: used struct type value where scalar is required
   if (p->gnutls_boot_parameters)
       ^

Shouldn't this be "if (! NILP (p->gnutls_boot_parameters))" ? With that
fix, 64bit mingw64 and 64bit cygwin w32 both build ok.

    AndyM

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 01 Feb 2016 03:36:13 +0100
> 
> Basically all it does is start a new thread that does getaddrinfo, and
> then stashes the response somewhere.

"Start a new thread"?  Do you mean with pthreads or somesuch?  IOW, do
we now have in Emacs application code that starts new threads?

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Mon, 01 Feb 2016 11:58:13 +0000
> 
> ../../src/process.c: In function ‘send_process’:
> ../../src/process.c:5907:7: error: used struct type value where scalar is required
>    if (p->gnutls_boot_parameters)
>        ^
> 
> Shouldn't this be "if (! NILP (p->gnutls_boot_parameters))" ? With that
> fix, 64bit mingw64 and 64bit cygwin w32 both build ok.

Did you configure with ----enable-check-lisp-object-type?

Re: Asynchronous DNS

[Andy Moreton]

On Mon 01 Feb 2016, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Mon, 01 Feb 2016 11:58:13 +0000
>> 
>> ../../src/process.c: In function ‘send_process’:
>> ../../src/process.c:5907:7: error: used struct type value where scalar is required
>>    if (p->gnutls_boot_parameters)
>>        ^
>> 
>> Shouldn't this be "if (! NILP (p->gnutls_boot_parameters))" ? With that
>> fix, 64bit mingw64 and 64bit cygwin w32 both build ok.
>
> Did you configure with ----enable-check-lisp-object-type?

Yes - I mentioned that in my previous mail.

    AndyM

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Mon, 01 Feb 2016 03:36:13 +0100
>> 
>> Basically all it does is start a new thread that does getaddrinfo, and
>> then stashes the response somewhere.
>
> "Start a new thread"?  Do you mean with pthreads or somesuch?  IOW, do
> we now have in Emacs application code that starts new threads?

I don't know, but does that matter?

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> Thanks. this gets further, but still has issues.
> On a mingw64 build (configured with --enable-check-lisp-object-type):
>
> ../../src/process.c: In function ‘send_process’:
> ../../src/process.c:5907:7: error: used struct type value where scalar is required
>    if (p->gnutls_boot_parameters)
>        ^
>
> Shouldn't this be "if (! NILP (p->gnutls_boot_parameters))" ?

Yup.  I've made that change and pushed it.

> With that fix, 64bit mingw64 and 64bit cygwin w32 both build ok.

Great!  Then I'll probably merge tomorrow if no new problems show up.

It would be great if a couple more people could build the branch and try
using it for network stuff and see whether anything breaks...  (Both on
glibc systems and other systems.)

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

Re: Asynchronous DNS

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 02 Feb 2016 02:54:32 +0100, Lars Ingebrigtsen <larsi@gnus.org> said:

> Andy Moreton <andrewjmoreton@gmail.com> writes:
>> Thanks. this gets further, but still has issues.
>> On a mingw64 build (configured with --enable-check-lisp-object-type):
>> 
>> ../../src/process.c: In function ‘send_process’:
>> ../../src/process.c:5907:7: error: used struct type value where scalar is required
>> if (p->gnutls_boot_parameters)
>> ^
>> 
>> Shouldn't this be "if (! NILP (p->gnutls_boot_parameters))" ?

> Yup.  I've made that change and pushed it.

There is another type error in check_for_dns.

static Lisp_Object
check_for_dns (Lisp_Object proc)
{
  struct Lisp_Process *p = XPROCESS (proc);
  Lisp_Object ip_addresses = Qnil;
  int ret = 0;

  /* Sanity check. */
  if (! p->dns_requests)
    return 1;
           ^ HERE !

Probably you can catch this kind of errors using
--enable-check-lisp-object-type.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Asynchronous DNS

[Lars Ingebrigtsen]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> There is another type error in check_for_dns.
>
> static Lisp_Object
> check_for_dns (Lisp_Object proc)
> {
>   struct Lisp_Process *p = XPROCESS (proc);
>   Lisp_Object ip_addresses = Qnil;
>   int ret = 0;
>
>   /* Sanity check. */
>   if (! p->dns_requests)
>     return 1;
>            ^ HERE !
>
> Probably you can catch this kind of errors using
> --enable-check-lisp-object-type.

Yup; thanks.  Fixed and pushed.

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

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Tue, 02 Feb 2016 02:15:42 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Lars Ingebrigtsen <larsi@gnus.org>
> >> Date: Mon, 01 Feb 2016 03:36:13 +0100
> >> 
> >> Basically all it does is start a new thread that does getaddrinfo, and
> >> then stashes the response somewhere.
> >
> > "Start a new thread"?  Do you mean with pthreads or somesuch?  IOW, do
> > we now have in Emacs application code that starts new threads?
> 
> I don't know, but does that matter?

Of course it does.  It's a major architecture change that we should
know about, and probably discuss before doing.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Tue, 02 Feb 2016 02:54:32 +0100
> Cc: emacs-devel@gnu.org
> 
> Great!  Then I'll probably merge tomorrow if no new problems show up.

Please don't merge before we complete the discussion of these changes.
For now, I'm not sure I agree with adding this feature as it is
implemented.

Thanks.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

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

For instance, if we run the current trunk under gdb, it says:


DISPLAY = :0
TERM = xterm-256color
Breakpoint 1 at 0x4eeef0: file emacs.c, line 354.
Temporary breakpoint 2 at 0x508260: init_sys_modes. (3 locations)
(gdb) run
Starting program: /home/larsi/src/emacs/trunk/src/emacs 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7fffe5cad700 (LWP 18679)]
[New Thread 0x7fffe4e86700 (LWP 18680)]
[New Thread 0x7fffdffff700 (LWP 18681)]

Running eww or Gnus will output a lot of

[New Thread 0x7fffdde17700 (LWP 18698)]
[New Thread 0x7fffdd3f0700 (LWP 18700)]
[New Thread 0x7fffdcbef700 (LWP 18701)]
[Thread 0x7fffdcbef700 (LWP 18701) exited]
[Thread 0x7fffdd3f0700 (LWP 18700) exited]

So...

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?

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Lars Ingebrigtsen <larsi@gnus.org>
>>> Date: Mon, 01 Feb 2016 03:36:13 +0100
>>> 
>>> Basically all it does is start a new thread that does getaddrinfo, and
>>> then stashes the response somewhere.
>>
>> "Start a new thread"?  Do you mean with pthreads or somesuch?  IOW, do
>> we now have in Emacs application code that starts new threads?
>
> I don't know, but does that matter?

What happens if background thread(s) are still alive (e.g. blocking,
waiting for a network response) and Emacs is killed? Will it wait for
the background threads to terminate or will it forcibly kill them?

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> What happens if background thread(s) are still alive (e.g. blocking,
> waiting for a network response) and Emacs is killed? Will it wait for
> the background threads to terminate or will it forcibly kill them?

I'm guessing they'll die along with Emacs.  That's what happens with
threads started by libraries currently, I think.

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alain Schneble <a.s@realize.ch> writes:
>
>> What happens if background thread(s) are still alive (e.g. blocking,
>> waiting for a network response) and Emacs is killed? Will it wait for
>> the background threads to terminate or will it forcibly kill them?
>
> I'm guessing they'll die along with Emacs.  That's what happens with
> threads started by libraries currently, I think.

If so, depending on how network resources are used, it may not release
them properly.  They /might/ be released while the process is being
stopped.  But will they be released in any case?  After all, it sounds a
bit awkward to rely on such a behavior even if it would just work.
Doesn't it?

Re: Asynchronous DNS

[Stefan Monnier]

> What problems do you see with stopping/starting threads in Emacs (that
> do no Lisp-related stuff)?

None, as long as the Emacs build notices that we're using threads.

> I'm assuming there are plenty of libraries that Emacs uses that
> already does this behind Emacs' back,

Indeed.  If you built with Gtk you'll use threads, and IIRC the same
holds for dbus and a few other libraries commonly linked with Emacs.

> and we don't seem to care...

Actually, we do, but AFAIK we don't need to know about each and every
thread, we just need to know that threading is used (IIRC, the issues we
need to handle have to do with signal delivery, making sure we run them
in the main Emacs/Elisp thread).


        Stefan

Re: Asynchronous DNS

[Eli Zaretskii]

[Sorry, sent this reposnse to the wrong message, resending.]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Tue, 02 Feb 2016 04:48:38 +0100
> 
> > 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.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Tue, 02 Feb 2016 08:25:47 -0500
> 
> > and we don't seem to care...
> 
> Actually, we do, but AFAIK we don't need to know about each and every
> thread, we just need to know that threading is used (IIRC, the issues we
> need to handle have to do with signal delivery, making sure we run them
> in the main Emacs/Elisp thread).

We need to know what Emacs code can be run in a separate thread,
because such code and changes in it should be audited specially.

Re: Asynchronous DNS

[Stefan Monnier]

>> > and we don't seem to care...
>> Actually, we do, but AFAIK we don't need to know about each and every
>> thread, we just need to know that threading is used (IIRC, the issues we
>> need to handle have to do with signal delivery, making sure we run them
>> in the main Emacs/Elisp thread).
> We need to know what Emacs code can be run in a separate thread,
> because such code and changes in it should be audited specially.

Indeed, the code run in a separate thread needs to be written with care,
but what I meant is that the rest of Emacs doesn't need to care (any
more, since we already took care of it for Gtk and friends).


        Stefan

Re: Asynchronous DNS

[Alain Schneble]

Alain Schneble <a.s@realize.ch> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> I'm guessing they'll die along with Emacs.  That's what happens with
>> threads started by libraries currently, I think.
>
> If so, depending on how network resources are used, it may not release
> them properly.  They /might/ be released while the process is being
> stopped.  But will they be released in any case?  After all, it sounds a
> bit awkward to rely on such a behavior even if it would just work.
> Doesn't it?

Well, I now see that the async dns lookup is initiated using
`getaddrinfo_a' with GAI_NOWAIT and what it does behind the scenes is
hidden.  As Florian pointed out, it uses threads under the cover.  But
maybe it's not relevant at all for this discussion.

But still, I wonder why there is no call to `gai_cancel' at all.  In
`wait_reading_process_output', there is the call to `check_for_dns'.
I would expect a call to `gai_cancel' somewhere in
`wait_reading_process_output' or the like to explicitly cancel the
corresponding pending async request(s), e.g. when cancelling with `C-g'
or when killing the process.  Am I missing something?  Or would that
just be a nicety that nobody cares about?

And please bear with me, it's the first time I look into `process.c'
(and c source code as well...).

Thanks!
Alain

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> If so, depending on how network resources are used, it may not release
> them properly.  They /might/ be released while the process is being
> stopped.  But will they be released in any case?  After all, it sounds a
> bit awkward to rely on such a behavior even if it would just work.
> Doesn't it?

That's not how things work.  If you exit a program, all resources are
released.  There's nothing magical about threads.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

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

Except the malloc thing, that just boils down to "you can't do anything
Lispy on the other threads" I think?  Which nobody has proposed, anyway.

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

No, I wrote that that's what getaddrinfo_a does, so we could just do
that ourselves and not have to rely on getaddrinfo_a.

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

Ok, but it's a problem when debugging in gdb, and not a problem for an
Emacs running without gdb?

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Please don't merge before we complete the discussion of these changes.
> For now, I'm not sure I agree with adding this feature as it is
> implemented.

What are your objections?

I just ran into a problem, though.  :-)

erc does the following on connection:

(make-network-process :name name :buffer  buffer
                      :host host :service service :nowait t)
...
(when (fboundp 'set-process-coding-system)
  (set-process-coding-system process 'raw-text))

That function starts with:

  CHECK_PROCESS (process);
  p = XPROCESS (process);
  if (p->infd < 0)
    error ("Input file descriptor of %s closed", SDATA (p->name));
  if (p->outfd < 0)
    error ("Output file descriptor of %s closed", SDATA (p->name));

And now, :nowait returns even before infd/outfd has been set, which
means the async DNS isn't as invisible to the user as I had hoped...

We could fix up the callers, of course, but that's a kinda yucky thing
to do.  Perhaps my original thought of having a separate :asynchronous
parameter to make-network-stream is a better idea for backwards
compatibility?  Or...  :nowait 'dns, perhaps.  Then the libraries that
want a fully async connection can say that explicitly by changing their
":nowait t"'s to ":nowait 'dns"?

Opinions?

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> Please don't merge before we complete the discussion of these changes.
>> For now, I'm not sure I agree with adding this feature as it is
>> implemented.
>
> What are your objections?

(They were posted in the related TLS bug thread that I read later.)

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

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

It doesn't seem to...  Just a lot of lines with

0x00007fffdedc6a40  0x00007fffdedd1b53  Yes (*)     /usr/lib/x86_64-linux-gnu/libtdb.so.1
0x00007fffdeb9add0  0x00007fffdebb145d  Yes (*)     /usr/lib/x86_64-linux-gnu/libvorbis.so.0
0x00007fffde990a70  0x00007fffde994c05  Yes (*)     /usr/lib/x86_64-linux-gnu/libogg.so.0
0x00007fffde78ae80  0x00007fffde78ccf9  Yes (*)     /usr/lib/x86_64-linux-gnu/gdk-pixbuf-2.0/2.10.0/loaders/libpixbufloader-png.so
0x00007fffde506bb0  0x00007fffde506fa9  Yes (*)     /usr/lib/x86_64-linux-gnu/gdk-pixbuf-2.0/2.10.0/loaders/libpixbufloader-svg.so
0x00007fffde197c10  0x00007fffde19880f  Yes (*)     /lib/x86_64-linux-gnu/libnss_mdns4_minimal.so.2
0x00007fffddf920a0  0x00007fffddf95463  Yes         /lib/x86_64-linux-gnu/libnss_dns.so.2
(*): Shared library is missing debugging information.


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

Re: Asynchronous DNS

[Yuri Khan]

On Wed, Feb 3, 2016 at 6:22 AM, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> That's not how things work.  If you exit a program, all resources are
> released.  There's nothing magical about threads.

If a program acquires a resource such as a lock file stored on a
remote machine, then gets killed, the remote resource might not be
released.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Wed, 03 Feb 2016 11:42:57 +1100
> 
> > 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.
> 
> Except the malloc thing, that just boils down to "you can't do anything
> Lispy on the other threads" I think?

I don't think "anything Lispy" is a useful definition, since almost
everything we do in Emacs on the C level is "Lispy" in some way.  And
a C programmer won't see too many "Lispy" things, I think.  E.g., QUIT
just calls longjmp, a standard C function.  Many functions that have
potentially long loops call QUIT inside the loop, and you cannot call
those in a non-main thread.  Assigning values to a global variable is
standard C practice, but there are some (quite a few, actually)
variables in Emacs which cannot be safely assigned from a non-main
thread, unless you introduce some synchronization mechanism.

> Which nobody has proposed, anyway.

I was just explaining why knowing that we are starting application
threads, and knowing what code these threads can run, is important.

> > 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".)
> 
> No, I wrote that that's what getaddrinfo_a does, so we could just do
> that ourselves and not have to rely on getaddrinfo_a.

If we introduce such a thread, its code needs to be carefully audited
for the above-mentioned gotchas.  E.g., passing to it a C pointer to a
contents of a Lisp string is probably unsafe.  It's not rocket science
to DTRT in this case (we already do that quite a lot on MS-Windows),
it just requires very careful programming and code review.

> >> 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.
> 
> Ok, but it's a problem when debugging in gdb, and not a problem for an
> Emacs running without gdb?

Only for the former, AFAIK.  GDB is complaining about a data structure
it builds, not about something the executable does.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: andrewjmoreton@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 03 Feb 2016 11:50:29 +1100
> 
> erc does the following on connection:
> 
> (make-network-process :name name :buffer  buffer
>                       :host host :service service :nowait t)
> ...
> (when (fboundp 'set-process-coding-system)
>   (set-process-coding-system process 'raw-text))
> 
> That function starts with:
> 
>   CHECK_PROCESS (process);
>   p = XPROCESS (process);
>   if (p->infd < 0)
>     error ("Input file descriptor of %s closed", SDATA (p->name));
>   if (p->outfd < 0)
>     error ("Output file descriptor of %s closed", SDATA (p->name));
> 
> And now, :nowait returns even before infd/outfd has been set, which
> means the async DNS isn't as invisible to the user as I had hoped...
> 
> We could fix up the callers, of course, but that's a kinda yucky thing
> to do.  Perhaps my original thought of having a separate :asynchronous
> parameter to make-network-stream is a better idea for backwards
> compatibility?  Or...  :nowait 'dns, perhaps.  Then the libraries that
> want a fully async connection can say that explicitly by changing their
> ":nowait t"'s to ":nowait 'dns"?

We could remove these sanity checks (in this and many other
process-related APIs), they are not strictly necessary, at least not
in that function, AFAICS.

But I think a better approach is to make all those APIs that check or
need a fully-functional process object to wait for DNS completion
before they proceed.  That would be fully backward-compatible, at the
price that the callers will need to be changed to actually take
advantage of this feature.  Which I think is a Good Thing, because for
any application code that wants the async DNS we need a human to
seriously consider all the implications and modify the code
accordingly.  Evidently, this isn't something that can be
transparently changed under the hood.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Yuri Khan <yuri.v.khan@gmail.com> writes:

> On Wed, Feb 3, 2016 at 6:22 AM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
>> That's not how things work.  If you exit a program, all resources are
>> released.  There's nothing magical about threads.
>
> If a program acquires a resource such as a lock file stored on a
> remote machine, then gets killed, the remote resource might not be
> released.

Programs can do many things.  I don't know what this has to do with
threads and sockets, though.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> If we introduce such a thread, its code needs to be carefully audited
> for the above-mentioned gotchas.  E.g., passing to it a C pointer to a
> contents of a Lisp string is probably unsafe.  It's not rocket science
> to DTRT in this case (we already do that quite a lot on MS-Windows),
> it just requires very careful programming and code review.

Sure.  But I think that for the getaddrinfo_a thing, it really is that
simple.  Everything is allocated and freed in the main thread, the
address lookup doesn't need to access anything Lispy, etc.  (This is how
the structure of the getaddrinfo_a library already works.)

>> Ok, but it's a problem when debugging in gdb, and not a problem for an
>> Emacs running without gdb?
>
> Only for the former, AFAIK.  GDB is complaining about a data structure
> it builds, not about something the executable does.

I see.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> We could remove these sanity checks (in this and many other
> process-related APIs), they are not strictly necessary, at least not
> in that function, AFAICS.

We could, and that would give us a better socket interface in the long
term, I think.  That is, no callers would have to care what level of
asynchronous socket we're setting up.  But I think that that's probably
a lot of work.

> But I think a better approach is to make all those APIs that check or
> need a fully-functional process object to wait for DNS completion
> before they proceed.  That would be fully backward-compatible, at the
> price that the callers will need to be changed to actually take
> advantage of this feature.  Which I think is a Good Thing, because for
> any application code that wants the async DNS we need a human to
> seriously consider all the implications and modify the code
> accordingly.  Evidently, this isn't something that can be
> transparently changed under the hood.

Yup.  I'll change make-network-address to only do async DNS when :nowait
is 'dns, I think...

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> If we introduce such a thread, its code needs to be carefully audited
>> for the above-mentioned gotchas.  E.g., passing to it a C pointer to a
>> contents of a Lisp string is probably unsafe.  It's not rocket science
>> to DTRT in this case (we already do that quite a lot on MS-Windows),
>> it just requires very careful programming and code review.
>
> Sure.  But I think that for the getaddrinfo_a thing, it really is that
> simple.  Everything is allocated and freed in the main thread, the
> address lookup doesn't need to access anything Lispy, etc.  (This is how
> the structure of the getaddrinfo_a library already works.)
>

Just for the record, getaddrinfo_a creates at least one detached worker
thread to synchronously call getaddrinfo.  If all threads are busy, it
will spawn new threads.  Idle workers will die eventually after some
period of inactivity.

So far so good.

>Alain Schneble <a.s@realize.ch> writes:
>
>But still, I wonder why there is no call to `gai_cancel' at all.

What I meant with this is that, when working with external resources,
the caller should explicitly account for the "unusual" (e.g. shutdown)
cases as well, explicitly, IMHO.  Given the case where any number of
async DNS requests are still pending and the (OS) process is to be
terminated, it should explicitly cancel all pending requests before its
last heartbeat.  For me, this sounds like best practice.  I understand
that it works properly without it in this async-DNS-feature case.  But
still -- maybe due to my zero knowledge about glibc -- I see the DNS
resolve library as a black box.  It could well have started let's say a
new OS process which does the resolve and which would survive our
"initiator" OS process if the requests are not explicitly cancelled
before termination.  One could argue that this would then be a bug, but
maybe not. I'm happy to learn that such things won't ever happen in
glibc by convention.

Furthermore, in your proposal, processes with pending DNS requests are
maintained in a separate list `dns_processes'.  If a LISP process gets
deleted by a call to `delete-process', and DNS requests are still
pending for it, it might be that `connect_network_socket' will be called
even though the process has been deleted.  Shouldn't
`deactivate_process' cancel any pending requests for that process?  Or
do we just forget about this probably "theoretical" case?

Alain

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> Given the case where any number of async DNS requests are still
> pending and the (OS) process is to be terminated, it should explicitly
> cancel all pending requests before its last heartbeat.  For me, this
> sounds like best practice.

It sounds like strange practice to me.  >"?  The OS will clean up
everything, no matter whether we're using threads or not.  If not, the
OS would be pretty fragile.

> Furthermore, in your proposal, processes with pending DNS requests are
> maintained in a separate list `dns_processes'.  If a LISP process gets
> deleted by a call to `delete-process', and DNS requests are still
> pending for it, it might be that `connect_network_socket' will be called
> even though the process has been deleted.

That sounds like a bug.  I'll get fixin'.

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> It sounds like strange practice to me.  >"?  The OS will clean up
> everything, no matter whether we're using threads or not.  If not, the
> OS would be pretty fragile.

Agreed with the last statement.  But in the general case, it could be
that the clean up the OS does will result in an application inconsistent
(external) state whereas an explicit cleanup won't as it may include
more processing/clean up.  But as it apparently does not apply to this
case, I'll just stop to elaborate on this and start to learn more about
the Emacs environment instead.  And I'll try to no longer waste your
time with this topic.  I already exhausted it too much with foolish
questions, I think... :)

>> Furthermore, in your proposal, processes with pending DNS requests are
>> maintained in a separate list `dns_processes'.  If a LISP process gets
>> deleted by a call to `delete-process', and DNS requests are still
>> pending for it, it might be that `connect_network_socket' will be called
>> even though the process has been deleted.
>
> That sounds like a bug.  I'll get fixin'.

Thanks!  In this regard, I was wondering if it is really worth to
maintain a separate `dns_processes' list.  Why not just loop over all
processes and try to get the requests for those where `p->dns_requests'
is not NULL?  Would that be too costly?

Thanks again
Alain

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Thu, 04 Feb 2016 13:22:29 +1100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > If we introduce such a thread, its code needs to be carefully audited
> > for the above-mentioned gotchas.  E.g., passing to it a C pointer to a
> > contents of a Lisp string is probably unsafe.  It's not rocket science
> > to DTRT in this case (we already do that quite a lot on MS-Windows),
> > it just requires very careful programming and code review.
> 
> Sure.  But I think that for the getaddrinfo_a thing, it really is that
> simple.

In that case, we don't launch threads of our own; libc does.  So
that's not the case I was talking about.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: andrewjmoreton@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 04 Feb 2016 13:25:11 +1100
> 
> > But I think a better approach is to make all those APIs that check or
> > need a fully-functional process object to wait for DNS completion
> > before they proceed.  That would be fully backward-compatible, at the
> > price that the callers will need to be changed to actually take
> > advantage of this feature.  Which I think is a Good Thing, because for
> > any application code that wants the async DNS we need a human to
> > seriously consider all the implications and modify the code
> > accordingly.  Evidently, this isn't something that can be
> > transparently changed under the hood.
> 
> Yup.  I'll change make-network-address to only do async DNS when :nowait
> is 'dns, I think...

But even then, an application that just specifies this and doesn't
make any other changes in the code structure might become broken.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> In this regard, I was wondering if it is really worth to maintain a
> separate `dns_processes' list.  Why not just loop over all processes
> and try to get the requests for those where `p->dns_requests' is not
> NULL?  Would that be too costly?

Is there a process list somewhere?  There's a file descriptor array, but
these processes don't have a file descriptor yet, so...

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > If we introduce such a thread, its code needs to be carefully audited
>> > for the above-mentioned gotchas.  E.g., passing to it a C pointer to a
>> > contents of a Lisp string is probably unsafe.  It's not rocket science
>> > to DTRT in this case (we already do that quite a lot on MS-Windows),
>> > it just requires very careful programming and code review.
>> 
>> Sure.  But I think that for the getaddrinfo_a thing, it really is that
>> simple.
>
> In that case, we don't launch threads of our own; libc does.  So
> that's not the case I was talking about.

I was talking about replacing the call to getaddrinfo_a with our own
thing, where we would spawn our own thread.  And that replacement would
be that simple, as I think I've said a few times now.  :-)

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Yup.  I'll change make-network-address to only do async DNS when :nowait
>> is 'dns, I think...
>
> But even then, an application that just specifies this and doesn't
> make any other changes in the code structure might become broken.

Yes, so applications that request async DNS would presumably know that
they've done so and program accordingly (i.e., wait until the state
changes to the appropriate state before talking to the process).

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

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  <emacs-devel@gnu.org>
> Date: Fri, 05 Feb 2016 13:08:59 +1100
> 
> Is there a process list somewhere?

See Vprocess_alist.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Fri, 05 Feb 2016 13:31:07 +1100
> 
> I was talking about replacing the call to getaddrinfo_a with our own
> thing, where we would spawn our own thread.  And that replacement would
> be that simple, as I think I've said a few times now.  :-)

I will agree or disagree when I see the code.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: andrewjmoreton@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 05 Feb 2016 13:32:25 +1100
> 
> > But even then, an application that just specifies this and doesn't
> > make any other changes in the code structure might become broken.
> 
> Yes, so applications that request async DNS would presumably know that
> they've done so and program accordingly (i.e., wait until the state
> changes to the appropriate state before talking to the process).

How can an application guess that requesting async DNS gets them much
more than they asked for?

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  <emacs-devel@gnu.org>
>> Date: Fri, 05 Feb 2016 13:08:59 +1100
>> 
>> Is there a process list somewhere?
>
> See Vprocess_alist.

Thanks.  I don't know how I missed it..  It's even the next-to-last
statement in make_process, that I thought I had read:

  Vprocess_alist = Fcons (Fcons (name, val), Vprocess_alist);


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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: andrewjmoreton@gmail.com,  emacs-devel@gnu.org
>> Date: Fri, 05 Feb 2016 13:32:25 +1100
>> 
>> > But even then, an application that just specifies this and doesn't
>> > make any other changes in the code structure might become broken.
>> 
>> Yes, so applications that request async DNS would presumably know that
>> they've done so and program accordingly (i.e., wait until the state
>> changes to the appropriate state before talking to the process).
>
> How can an application guess that requesting async DNS gets them much
> more than they asked for?

It's in the documentation.

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

Re: Asynchronous DNS

[Alain Schneble]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  <emacs-devel@gnu.org>
>> Date: Fri, 05 Feb 2016 13:08:59 +1100
>> 
>> Is there a process list somewhere?
>
> See Vprocess_alist.

And the related FOR_EACH_PROCESS macro.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

I kinda think we've gotten as far as we're going to get in this
discussion, and that it's time to merge.

For people who haven't been following this: There's a new value to
:nowait to make-network-process.  If it's `dns', then the DNS is
asynchronous, and there are other rules as how you have to interact with
the process object.  (In short, don't talk to it until it switches
status to "connected".)

There should be no impact on any existing use.  It should be 100%
backward compatible.

(Unless I've created bugs somewhere while refactoring the code, but
that's surely impossible.)

After merging, we can continue to examine the details of the
implementation, and change anything we think doesn't work.

If there are no bugs to be found in the refactored code, I will then
proceed to alter the TLS negotiation code to make it, too, fully
asynchronous (i.e., absolutely no user-visible hangs when calling things
like `(url-retrieve "https://very-slow-site.com/")').

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

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sat, 06 Feb 2016 18:49:43 +1100
> 
> I kinda think we've gotten as far as we're going to get in this
> discussion, and that it's time to merge.

I'm sorry, but I disagree.  I think the APIs that expect a fully
functional process object should wait for the DNS resolution to
complete, before they do anything.

> There should be no impact on any existing use.  It should be 100%
> backward compatible.

But it isn't, as was demonstrated by ERC.  And it cannot be 100%
backward compatible, unless the above-mentioned waiting is added to
the affected APIs.

> If there are no bugs to be found in the refactored code, I will then
> proceed to alter the TLS negotiation code to make it, too, fully
> asynchronous (i.e., absolutely no user-visible hangs when calling things
> like `(url-retrieve "https://very-slow-site.com/")').

As I made it clear in the related discussion in bug#22493, I'm opposed
to that change, in the way it is proposed, FWIW.

Re: Asynchronous DNS

[Alain Schneble]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Sat, 06 Feb 2016 18:49:43 +1100
>> 
>> I kinda think we've gotten as far as we're going to get in this
>> discussion, and that it's time to merge.
>
> I'm sorry, but I disagree.  I think the APIs that expect a fully
> functional process object should wait for the DNS resolution to
> complete, before they do anything.

FWIW, the process-send-* functions do not wait for an async connection
to complete either, even without async-DNS-feature turned on.  They
throw an error if called before the async connection completed, IIUC.
And the documentation does not mention anything about this behavior.

It seems to me like it is a matter of definition to say which functions
should work properly also before an async connection is established and
which not.

If backward compatibility were not an issue, we could also adopt a more
restrictive approach where errors would be thrown in many of the
functions accepting a (async socket) Lisp process and requiring a valid
infd/outfd before async connection completion.  Documentation of
`make-network-process' could say something like "if :nowait is t,
process returned shall not be used in any call before async connection
has completed.  Consider performing any required process configuration
withing the SENTINEL".  Oops, just realized that this is what Lars more
or less put in the doc string for the 'dns case...

Just as an overview, here is an extract of functions relying on a valid
infd/outfd and hence would be candidates that could either throw errors
or, as you suggested, synchronously wait for the DNS resolution to
complete.  There are quite a few though:

`process-contact', `set-process-filter', `set-process-filter-multibyte',
`set-process-window-size', `set-network-process-option',
`set-process-coding-system', `process-filter-multibyte-p',
`process-datagram-address', `set-process-datagram-address',
`process-send-region', `process-send-string', `process-send-eof'

>> There should be no impact on any existing use.  It should be 100%
>> backward compatible.
>
> But it isn't, as was demonstrated by ERC.  And it cannot be 100%
> backward compatible, unless the above-mentioned waiting is added to
> the affected APIs.

I think what Lars meant was that with :nowait to t, client code should
be 100% backward compatible, as async connection establishment is still
handled the same way internally.

Only when :nowait is 'dns, the new async DNS resolution is activated.
And only in this case, we might run into /new/ issues with calls to
functions accepting a Lisp process, such as the issue revealed by ERC
before Lars introduced 'dns.

On one hand, I see the practical approach with the 'dns async mode to
circumvent these compatibility issues.  On the other hand, I don't think
it's a good choice from a interface/usage point of view.  I think it is
not clear for a user why she has to care about whether both async DNS
and async socket connection or only the latter shall be used and why the
process behavior is different.  This distinction only makes sense with
the knowledge of how it's implemented internally, I think.

I guess changing the semantics of :nowait t, to say that clients shall
not use the returned process before connection is established, is out of
discussion, right?  Or do you think that this would even be a ridiculous
restriction (even without the backward compatibility issue)?

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Sat, 06 Feb 2016 18:49:43 +1100
>> 
>> I kinda think we've gotten as far as we're going to get in this
>> discussion, and that it's time to merge.
>
> I'm sorry, but I disagree.  I think the APIs that expect a fully
> functional process object should wait for the DNS resolution to
> complete, before they do anything.

I don't know what you mean by that.  That is exactly what the API does.

>> There should be no impact on any existing use.  It should be 100%
>> backward compatible.
>
> But it isn't, as was demonstrated by ERC.  And it cannot be 100%
> backward compatible, unless the above-mentioned waiting is added to
> the affected APIs.

Again, it is not demonstrated by ERC.  ERC works fine with the new
refactoring.

I'm beginning to suspect that you're kinda just skimming my messages.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> On one hand, I see the practical approach with the 'dns async mode to
> circumvent these compatibility issues.  On the other hand, I don't think
> it's a good choice from a interface/usage point of view.  I think it is
> not clear for a user why she has to care about whether both async DNS
> and async socket connection or only the latter shall be used and why the
> process behavior is different.  This distinction only makes sense with
> the knowledge of how it's implemented internally, I think.

Perhaps 'dns is the wrong symbol to use for :nowait, since it isn't
really about DNS, but getting a fully asynchronous connection.  That is,
:nowait t gives us a fully set up socket, really (seen from the
application point of view).  Perhaps :nowait 'immediate or something
would make more sense than :nowait 'dns, which is, after all, just an
implementation detail.

> I guess changing the semantics of :nowait t, to say that clients shall
> not use the returned process before connection is established, is out of
> discussion, right?

Yes.

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Perhaps 'dns is the wrong symbol to use for :nowait, since it isn't
> really about DNS, but getting a fully asynchronous connection.  That is,
> :nowait t gives us a fully set up socket, really (seen from the
> application point of view).  Perhaps :nowait 'immediate or something
> would make more sense than :nowait 'dns, which is, after all, just an
> implementation detail.

Agreed.  A name reflecting more the level of "asynchronicity" would be a
better choice and less misleading, I think.

But as said, even better would be a solution where this distinction is
not necessary at all, IMHO.  But you tried it out and the only feasible
approach I see would be what Eli Zaretskii proposed, IIUC, to block and
synchronously wait for DNS resolve completion and socket initialization
in all the functions requiring a Lisp process having "valid" infd and/or
outfd set.  OTOH, that sounds quite invasive to those functions, doesn't
it?

>> I guess changing the semantics of :nowait t, to say that clients shall
>> not use the returned process before connection is established, is out of
>> discussion, right?
>
> Yes.

Alright, thanks for clarifying this.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Alain Schneble <a.s@realize.ch>
> CC: Lars Ingebrigtsen <larsi@gnus.org>, <emacs-devel@gnu.org>
> Date: Sun, 7 Feb 2016 01:34:14 +0100
> 
> >> I kinda think we've gotten as far as we're going to get in this
> >> discussion, and that it's time to merge.
> >
> > I'm sorry, but I disagree.  I think the APIs that expect a fully
> > functional process object should wait for the DNS resolution to
> > complete, before they do anything.
> 
> FWIW, the process-send-* functions do not wait for an async connection
> to complete either, even without async-DNS-feature turned on.  They
> throw an error if called before the async connection completed, IIUC.

That's not my reading of the code.  Perhaps I'm missing something.
Could you tell the details -- where do you see this?

What I see is this: process-send-string calls send_process, which
signals an error if either the status of the process object is other
than 'run' or its output file descriptor is invalid.  But
make-network-process takes care to arrange for both of these: it calls
make_process, which sets the process status to 'run' and sets up its
output file descriptor to a valid value (the socket we got from the
call to 'socket').  It does that even if the connection is
asynchronous and has not yet completed.  So I don't expect an error in
this scenario with the existing code.

If the connection is asynchronous, and is not yet completed, then
send_process will wait in a loop, because the write to the socket will
return EWOULDBLOCK or EAGAIN.

Did I miss something?

> And the documentation does not mention anything about this behavior.

Well, if I'm right, it shouldn't ;-)

> It seems to me like it is a matter of definition to say which functions
> should work properly also before an async connection is established and
> which not.

I definitely hope that is not true.  We couldn't have reliable network
connections if it were true.

> If backward compatibility were not an issue, we could also adopt a more
> restrictive approach where errors would be thrown in many of the
> functions accepting a (async socket) Lisp process and requiring a valid
> infd/outfd before async connection completion.

I think this would make network programming in Emacs a nuisance.

> Documentation of `make-network-process' could say something like "if
> :nowait is t, process returned shall not be used in any call before
> async connection has completed.  Consider performing any required
> process configuration withing the SENTINEL".  Oops, just realized
> that this is what Lars more or less put in the doc string for the
> 'dns case...

Please consider the plight of a programmer who needs to implement such
a connection.  It would be much more complex and hard to get right
than what we have today.  E.g., look at smtpmail.el: it opens the
network stream, sets up the process filter and encoding, and
immediately proceeds to sending the HELO command to the server.
Imagine how much more complex this simple job would be if it needed to
jump through hoops as you described above.

> Just as an overview, here is an extract of functions relying on a valid
> infd/outfd and hence would be candidates that could either throw errors
> or, as you suggested, synchronously wait for the DNS resolution to
> complete.  There are quite a few though:

They all should call a single function that waits calling gai_error
until the connection completes, and signals an error if it completes
with a failure indication.  That's a very simple and straightforward
addition to the preamble of all those functions, and it should fix all
those problems in a way that is 100% backward compatible.

> >> There should be no impact on any existing use.  It should be 100%
> >> backward compatible.
> >
> > But it isn't, as was demonstrated by ERC.  And it cannot be 100%
> > backward compatible, unless the above-mentioned waiting is added to
> > the affected APIs.
> 
> I think what Lars meant was that with :nowait to t, client code should
> be 100% backward compatible, as async connection establishment is still
> handled the same way internally.

Yes, I understand.  But I don't think this is a proper way to be
backward compatible.  With my suggestion, code remains backward
compatible even if it uses :nowait.  This is better, IMO, because it
leaves more applications backward compatible.  For example, imagine
that :nowait comes from some higher level, so the level that arranges
the connection has no idea whether it needs to do the simple thing or
the complex one.  What Lars suggested means that every application
that doesn't pass literal parameters to make-network-process will now
have to analyze the parameters passed by the caller, to see if :nowait
is there and what is its value.  I don't expect package maintainers to
love this.

> Only when :nowait is 'dns, the new async DNS resolution is activated.
> And only in this case, we might run into /new/ issues with calls to
> functions accepting a Lisp process, such as the issue revealed by ERC
> before Lars introduced 'dns.

I think the solution I suggest will provide backward compatibility
even if :nowait is 'dns.  So the new issues are avoided.

> On one hand, I see the practical approach with the 'dns async mode to
> circumvent these compatibility issues.  On the other hand, I don't think
> it's a good choice from a interface/usage point of view.  I think it is
> not clear for a user why she has to care about whether both async DNS
> and async socket connection or only the latter shall be used and why the
> process behavior is different.  This distinction only makes sense with
> the knowledge of how it's implemented internally, I think.

Yes, and so my suggestion is better, I think, as it avoids all of
that.  An application that didn't make the necessary changes will not
be able to take the full advantage of the async DNS, but it will still
work.

> I guess changing the semantics of :nowait t, to say that clients shall
> not use the returned process before connection is established, is out of
> discussion, right?  Or do you think that this would even be a ridiculous
> restriction (even without the backward compatibility issue)?

Yes, that'd be a terrible nuisance, IMO.  Users and Lisp programmers
will hate us if we do that.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Sun, 07 Feb 2016 12:35:22 +1100
> 
> >> There should be no impact on any existing use.  It should be 100%
> >> backward compatible.
> >
> > But it isn't, as was demonstrated by ERC.  And it cannot be 100%
> > backward compatible, unless the above-mentioned waiting is added to
> > the affected APIs.
> 
> Again, it is not demonstrated by ERC.  ERC works fine with the new
> refactoring.

I would like us to avoid the need for refactoring.  With my
suggestion, the refactoring would only be needed if the application
wants to take full advantage of the async DNS resolution, but it will
still work correctly (albeit with some delays) if no refactoring was
done.

> I'm beginning to suspect that you're kinda just skimming my messages.

I'm sorry I caused this impression, because it's definitely not what
happens.  I carefully read everything that you write, in this thread
or in any other.

Re: Asynchronous DNS

[John Wiegley]

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]

>>>>> Lars Ingebrigtsen <larsi@gnus.org> writes:

> I kinda think we've gotten as far as we're going to get in this discussion,
> and that it's time to merge.

Hi Lars,

Given that there is disagreement from a main developer on this, I ask that
your change remain in a feature branch until we've had more time to reach
consensus, and to hear from others (some of whom I've copied directly, to
better ensure they receive this).

I do appreciate the contribution, and think asynchronous DNS is a valuable
addition, but it hasn't passed the litmus test yet for merging toward the next
release. If agreement can be reached before the next freeze, that's great;
otherwise, it will have to wait until later.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 629 bytes --]

Re: Asynchronous DNS

[Alain Schneble]

Eli Zaretskii <eliz@gnu.org> writes:

> That's not my reading of the code.  Perhaps I'm missing something.
> Could you tell the details -- where do you see this?

The probability that I'm missing something is infinitely larger ;)  And
at the same time I'm infinitely thankful to you and Lars for taking the
time to read my messages so thoroughly.

> What I see is this: process-send-string calls send_process, which
> signals an error if either the status of the process object is other
> than 'run' or its output file descriptor is invalid.  But
> make-network-process takes care to arrange for both of these: it calls
> make_process, which sets the process status to 'run' and sets up its
> output file descriptor to a valid value (the socket we got from the
> call to 'socket').

Up to this point, I fully agree.

>                      It does that even if the connection is
> asynchronous and has not yet completed.  So I don't expect an error in
> this scenario with the existing code.

Well, there's this code a few lines after p->outfd has been set, in
connect_network_socket:

  if (p->is_non_blocking_client)
    {
      /* We may get here if connect did succeed immediately.  However,
	 in that case, we still need to signal this like a non-blocking
	 connection.  */
      pset_status (p, Qconnect);
      if (!FD_ISSET (inch, &connect_wait_mask))
	{
	  FD_SET (inch, &connect_wait_mask);
	  FD_SET (inch, &write_mask);
	  num_pending_connects++;
	}
    }
  else

To me this reads like the process status is set to Qconnect until the
asynchronous connection is fully set up.  Near the end of
wait_reading_process_output, the status is set to Qrun again just before
invoking the sentinel function:

  pset_status (p, Qrun);
  ...
  exec_sentinel (proc, build_string ("open\n"));

> If the connection is asynchronous, and is not yet completed, then
> send_process will wait in a loop, because the write to the socket will
> return EWOULDBLOCK or EAGAIN.

This I see, but only if the above mentioned Qconnect state wouldn't be
there.

> Did I miss something?

Am I misinterpreting it?

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Alain Schneble <a.s@realize.ch>
> CC: Eli Zaretskii <eliz@gnu.org>, <emacs-devel@gnu.org>
> Date: Sun, 7 Feb 2016 12:41:08 +0100
> 
> But as said, even better would be a solution where this distinction is
> not necessary at all, IMHO.  But you tried it out and the only feasible
> approach I see would be what Eli Zaretskii proposed, IIUC, to block and
> synchronously wait for DNS resolve completion and socket initialization
> in all the functions requiring a Lisp process having "valid" infd and/or
> outfd set.  OTOH, that sounds quite invasive to those functions, doesn't
> it?

Why is it invasive?  A single call to a single function at the
beginning of each of those functions is all that's needed.  And if the
DNS resolution is already completed, that call will be a no-op.

Re: Asynchronous DNS

[Alain Schneble]

Eli Zaretskii <eliz@gnu.org> writes:

> Why is it invasive?  A single call to a single function at the
> beginning of each of those functions is all that's needed.  And if the
> DNS resolution is already completed, that call will be a no-op.

True.  I'm sorry, invasive maybe is the wrong term.  What I meant was
that all these functions will have to know about this new aspect that is
very specific to network processes/DNS resolving.  And I had the
impression that some of them (e.g. `process-send-eof') already have to
deal with quite a lot of different aspects.  But this is just the
subjective feeling of a novice.  I should have mentioned it.  It may be
of no importance at all.

Besides that, I think what you proposed would be an elegant solution.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

John Wiegley <jwiegley@gmail.com> writes:

> I do appreciate the contribution, and think asynchronous DNS is a valuable
> addition, but it hasn't passed the litmus test yet for merging toward the next
> release. If agreement can be reached before the next freeze, that's great;
> otherwise, it will have to wait until later.

Well, it's not for the next release, it's for the one after that...
(Unless by "next" you don't mean "the first".  :-))

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> Agreed.  A name reflecting more the level of "asynchronicity" would be a
> better choice and less misleading, I think.

:nowait 'immediate has a nice ring to it...

> But as said, even better would be a solution where this distinction is
> not necessary at all, IMHO.  But you tried it out and the only feasible
> approach I see would be what Eli Zaretskii proposed, IIUC, to block and
> synchronously wait for DNS resolve completion and socket initialization
> in all the functions requiring a Lisp process having "valid" infd and/or
> outfd set.  OTOH, that sounds quite invasive to those functions, doesn't
> it?

It would be better if the application code didn't have to care, but that
means that either all those functions will have to block (getting us
back to square one, basically), or they'll have to set up a queue of
pending actions to be taken, which I think is rather too ambitious. 

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Please consider the plight of a programmer who needs to implement such
> a connection.  It would be much more complex and hard to get right
> than what we have today.  E.g., look at smtpmail.el: it opens the
> network stream, sets up the process filter and encoding, and
> immediately proceeds to sending the HELO command to the server.
> Imagine how much more complex this simple job would be if it needed to
> jump through hoops as you described above.

Ideally, when you're sending mail with smtpmail.el, it would all happen
without Emacs being blocked, right?  So that the user can go on reading
the next email.

But that can't be done without doing major changes to smtpmail.el.  Way
errors are propagated back to the callers would have to change, for
instance.

I guess what I'm saying is this: If you're fine blocking Emacs, then you
don't use :nowait 'immediate.  Then everything happens one step at a
time (DNS, connect, TLS, EHLO, QUIT).

If you wish to make things happen "in the background", then you will
have to change your application in major ways.  Making one of those
things the rule "you have to wait until the process changes state to
"connected" is an insignificant part of that rewrite.

> They all should call a single function that waits calling gai_error
> until the connection completes, and signals an error if it completes
> with a failure indication.

Then you're back to making a blocking connection, which is something
that somebody who's said :nowait 'immediate would not want.  So I don't
really see the point here.  It's better to just have those functions
signal an error, I think, which is what they currently do...

> Yes, I understand.  But I don't think this is a proper way to be
> backward compatible.  With my suggestion, code remains backward
> compatible even if it uses :nowait.  This is better, IMO, because it
> leaves more applications backward compatible.  For example, imagine
> that :nowait comes from some higher level, so the level that arranges
> the connection has no idea whether it needs to do the simple thing or
> the complex one.  What Lars suggested means that every application
> that doesn't pass literal parameters to make-network-process will now
> have to analyze the parameters passed by the caller, to see if :nowait
> is there and what is its value.  I don't expect package maintainers to
> love this.

Hm...  I don't understand.  An application that wants 'immediate will
just say ":nowait 'immediate".  The only thing that examines the :nowait
parameter (to distinguish 'immediate, nil and non-nil) is
make-network-process.

Unless I'm missing something.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Again, it is not demonstrated by ERC.  ERC works fine with the new
>> refactoring.
>
> I would like us to avoid the need for refactoring.  With my
> suggestion, the refactoring would only be needed if the application
> wants to take full advantage of the async DNS resolution, but it will
> still work correctly (albeit with some delays) if no refactoring was
> done.

By "refactoring" I just mean "the rewrite of make_network_process that
turned it from one 700-line soup of interconnected code into two 350
line functions of less interconnected code".

There are no functional differences.  (Well, there shouldn't be.)

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> It would be better if the application code didn't have to care, but that
> means that either all those functions will have to block (getting us
> back to square one, basically), or they'll have to set up a queue of
> pending actions to be taken, which I think is rather too ambitious. 

And, by the way, as I think I've said before: The following code will
give a backtrace today:

         (setq proc (make-network-process :name "foo"
                                     :buffer (generate-new-buffer "*foo*")
                                     :host "localhost"
                                     :nowait t
                                     :service port)))
  (process-send-string proc "echo bar")

with

Process foo not running

because it's still in 'connect.

For even more asynchronicity in the :nowait 'immediate case, there are
other functions that will also fail, like set-process-coding-system,
that do not fail with :nowait t.  And I think that's fine.  When you're
doing async programming, you have to adhere to the rules provided by the
asynchronous sockets, and that you've explicitly asked for.

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alain Schneble <a.s@realize.ch> writes:
>
>> Agreed.  A name reflecting more the level of "asynchronicity" would be a
>> better choice and less misleading, I think.
>
> :nowait 'immediate has a nice ring to it...

...yes, indeed :)

>> But as said, even better would be a solution where this distinction is
>> not necessary at all, IMHO.  But you tried it out and the only feasible
>> approach I see would be what Eli Zaretskii proposed, IIUC, to block and
>> synchronously wait for DNS resolve completion and socket initialization
>> in all the functions requiring a Lisp process having "valid" infd and/or
>> outfd set.  OTOH, that sounds quite invasive to those functions, doesn't
>> it?
>
> It would be better if the application code didn't have to care, but that
> means that either all those functions will have to block (getting us
> back to square one, basically), ...

Well, back to square /two/ I would say, depending on how you arrange the
squares ;) Because application code would just loose the
async-DNS-resolve benefits.  As most of the functions such as
`set-process-coding-system', that would need to block, require only an
initialized socked (and not really a connected one).  And if the author
of the code doesn't care, he just accepted that he wrote semi-optimized
async code -- but code that actually works.  This doesn't sound wrong to
me.  The only thing that could happen is that he might not be aware of it.

>                             ... or they'll have to set up a queue of
> pending actions to be taken, which I think is rather too ambitious. 

Yes, that would introduce a new level of complexity that feels like it
isn't worth it.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> Well, back to square /two/ I would say, depending on how you arrange the
> squares ;) Because application code would just loose the
> async-DNS-resolve benefits.  As most of the functions such as
> `set-process-coding-system', that would need to block, require only an
> initialized socked (and not really a connected one).  And if the author
> of the code doesn't care, he just accepted that he wrote semi-optimized
> async code -- but code that actually works.  This doesn't sound wrong to
> me. 

Yeah, that's true.  It's less fragile.  But I wonder whether being that
application friendly can sometimes be a disservice?  If you're an
application writer, trying to be as async as possible (that is, after
all, why you're using :nowait 'immediate), would you prefer that random
functions block without telling you, or that they error out?

Hm...  I don't know what I'd prefer, really...  *ponder*

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alain Schneble <a.s@realize.ch> writes:
>
> Yeah, that's true.  It's less fragile.  But I wonder whether being that
> application friendly can sometimes be a disservice?  ...

Oh yes, indeed, it can be.  I'm with you.

>                                                 ...  If you're an
> application writer, trying to be as async as possible (that is, after
> all, why you're using :nowait 'immediate), would you prefer that random
> functions block without telling you, or that they error out?

I think the point here is that with these blocking calls in place, we
wouldn't really need a separate :nowait 'immediate, just the existing
:nowait t.  And IIUC, this is also what Eli Zaretskii proposes.  With
this model, :nowait t really means: I request as much async behavior as
possible.  But if code is not arranged in an async fashion, then well,
you might not get full async behavior.  It's your fault, but we won't
signal an error.

> Hm...  I don't know what I'd prefer, really...  *ponder*

*ponder-in-circles*

Re: Asynchronous DNS

[Andreas Schwab]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> It would be better if the application code didn't have to care, but that
> means that either all those functions will have to block (getting us
> back to square one, basically), or they'll have to set up a queue of
> pending actions to be taken, which I think is rather too ambitious. 

There's still the benefit of blocking in the Emacs event loop instead of
a system call, with the former Emacs can still run other process
filters.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Asynchronous DNS

[Alain Schneble]

Andreas Schwab <schwab@suse.de> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> It would be better if the application code didn't have to care, but that
>> means that either all those functions will have to block (getting us
>> back to square one, basically), or they'll have to set up a queue of
>> pending actions to be taken, which I think is rather too ambitious. 
>
> There's still the benefit of blocking in the Emacs event loop instead of
> a system call, with the former Emacs can still run other process
> filters.

That sounds interesting, but would it really be feasible to call out to
the event loop while in the blocking call?  I doubt so.  My
understanding was that calls to this new blocking function e.g. in
`set-process-coding-system' would really block the thread e.g. by a call
to gai_suspend.  Or didn't you refer to this "blocking function concept"
in your remark?

Re: Asynchronous DNS

[Andreas Schwab]

Alain Schneble <a.s@realize.ch> writes:

> Andreas Schwab <schwab@suse.de> writes:
>
>> Lars Ingebrigtsen <larsi@gnus.org> writes:
>>
>>> It would be better if the application code didn't have to care, but that
>>> means that either all those functions will have to block (getting us
>>> back to square one, basically), or they'll have to set up a queue of
>>> pending actions to be taken, which I think is rather too ambitious. 
>>
>> There's still the benefit of blocking in the Emacs event loop instead of
>> a system call, with the former Emacs can still run other process
>> filters.
>
> That sounds interesting, but would it really be feasible to call out to
> the event loop while in the blocking call?

That's what accept-process-output does.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Asynchronous DNS

[Alain Schneble]

Andreas Schwab <schwab@suse.de> writes:

> Alain Schneble <a.s@realize.ch> writes:
>
>> Andreas Schwab <schwab@suse.de> writes:
>>
>> That sounds interesting, but would it really be feasible to call out to
>> the event loop while in the blocking call?
>
> That's what accept-process-output does.

True, but my assumption was that it would be illegal to call
`accept-process-output' say, for example, from within
`set-process-coding-system' for the sake of waiting for the DNS resolve
to complete as it might have unwanted side effects.  But I might be
wrong.

Re: Asynchronous DNS

[Andreas Schwab]

Alain Schneble <a.s@realize.ch> writes:

> True, but my assumption was that it would be illegal to call
> `accept-process-output' say, for example, from within
> `set-process-coding-system' for the sake of waiting for the DNS resolve
> to complete as it might have unwanted side effects.

You have to wait for the completion event in some way.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Asynchronous DNS

[John Wiegley]

>>>>> Lars Ingebrigtsen <larsi@gnus.org> writes:

> Ideally, when you're sending mail with smtpmail.el, it would all happen
> without Emacs being blocked, right? So that the user can go on reading the
> next email.

In GNU ELPA, the async package uses async.el to make smtpmail easily
asynchronous, without any changes to smtpmail.el.

I mentioned async.el before in connection in improving asynchronicity in Gnus,
but I don't think you responded. Would you mind giving me your comments as to
its applicability

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  <emacs-devel@gnu.org>
> Date: Mon, 08 Feb 2016 18:52:55 +1100
> 
> > Well, back to square /two/ I would say, depending on how you arrange the
> > squares ;) Because application code would just loose the
> > async-DNS-resolve benefits.  As most of the functions such as
> > `set-process-coding-system', that would need to block, require only an
> > initialized socked (and not really a connected one).  And if the author
> > of the code doesn't care, he just accepted that he wrote semi-optimized
> > async code -- but code that actually works.  This doesn't sound wrong to
> > me. 
> 
> Yeah, that's true.  It's less fragile.  But I wonder whether being that
> application friendly can sometimes be a disservice?  If you're an
> application writer, trying to be as async as possible (that is, after
> all, why you're using :nowait 'immediate), would you prefer that random
> functions block without telling you, or that they error out?

I don't think we should punish users of those packages and their
developers by breaking their code.  If they didn't get their act
together, their code will not enjoy the advantages of asynchronicity,
but it should still work, IMO.  Teaching by punishment rarely works,
IME, certainly not in a community such as this one.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Mon, 08 Feb 2016 13:05:19 +1100
> 
> >> Again, it is not demonstrated by ERC.  ERC works fine with the new
> >> refactoring.
> >
> > I would like us to avoid the need for refactoring.  With my
> > suggestion, the refactoring would only be needed if the application
> > wants to take full advantage of the async DNS resolution, but it will
> > still work correctly (albeit with some delays) if no refactoring was
> > done.
> 
> By "refactoring" I just mean "the rewrite of make_network_process that
> turned it from one 700-line soup of interconnected code into two 350
> line functions of less interconnected code".

I'm talking about refactoring in the applications that _use_
make-network-process.  It's their refactoring that I would like us to
avoid, if possible.  IOW, let the maintainers of those applications
decide how deeply they want to refactor, and at what time schedule,
instead of requiring them to make a binary all-or-nothing choice,
which might be a price that is too heavy for them to pay.  ("Them"
here might mean us as well: we do have in Emacs quite a few features
that build elaborate applications on top of network APIs.)

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Alain Schneble <a.s@realize.ch>,  emacs-devel@gnu.org
> Date: Mon, 08 Feb 2016 13:03:44 +1100
> 
> Hm...  I don't understand.  An application that wants 'immediate will
> just say ":nowait 'immediate".  The only thing that examines the :nowait
> parameter (to distinguish 'immediate, nil and non-nil) is
> make-network-process.

These two places could be many levels apart.  The place where
'immediate' is used and the programmer who uses it might not be aware
of all of the implications.  E.g., the same set of parameters could be
used for several different series of calls to network-related APIs.

What I suggest is a way to let applications still work even if they
didn't make all the refactoring, or if the place to refactor is out of
their control, or whatever.

Re: Asynchronous DNS

[Rasmus]

John Wiegley <jwiegley@gmail.com> writes:

>>>>>> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Ideally, when you're sending mail with smtpmail.el, it would all happen
>> without Emacs being blocked, right? So that the user can go on reading the
>> next email.
>
> In GNU ELPA, the async package uses async.el to make smtpmail easily
> asynchronous, without any changes to smtpmail.el.

Are you sure?  Last I tried, I think I needed this piece of advice to get
it working.  Even then it was too unreliable for real world usage (as I
recall sometimes I would think emails would have been sent but they
weren’t).

Rasmus

(with-eval-after-load 'message
  (when (require 'smtpmail-async nil t)

    (defcustom rasmus/message-async nil
      "Should messages be send asynchronously?")
    
    (defun async-message-multi-smtp-send-mail (orig-fun &rest args)
      "Make `message-multi-smtp-send-mail' use async-smtp.el"
      (if rasmus/message-async
          (cl-letf (((symbol-function 'message-smtpmail-send-it)
                     #'async-smtpmail-send-it)
                    ;; I don't know if the following function mapping is
                    ;; necessary, but it's what is called from
                    ;; `message-multi-smtp-send-mail', but not from
                    ;; `async-smtpmail-send-it'.
                    ((symbol-function 'smtpmail-send-it)
                     #'message-smtpmail-send-it))
            (apply orig-fun args))
        (apply orig-fun args)))

    (advice-add 'message-multi-smtp-send-mail :around
                #'async-message-multi-smtp-send-mail)))

-- 
Together we will make the possible totalllly impossible!

Re: Asynchronous DNS

[Lars Ingebrigtsen]

John Wiegley <jwiegley@gmail.com> writes:

> I mentioned async.el before in connection in improving asynchronicity in Gnus,
> but I don't think you responded. Would you mind giving me your comments as to
> its applicability

Well, I think async.el is fine as a tool to do async computation, but
when we're dealing with network sockets, we already have the tools to do
async handling in a single Emacs, which simplifies some things.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> I think the point here is that with these blocking calls in place, we
> wouldn't really need a separate :nowait 'immediate, just the existing
> :nowait t.  And IIUC, this is also what Eli Zaretskii proposes.  With
> this model, :nowait t really means: I request as much async behavior as
> possible.  But if code is not arranged in an async fashion, then well,
> you might not get full async behavior.  It's your fault, but we won't
> signal an error.

If we can identify all the places where we have to insert these
blockers, then having :nowait t be as async as possible, then that would
be nice.  I kinda feel that might not be realistic.  But I may be
wrong.  I mean, there aren't a hundred process-related built-in
functions...  Your list was just in the tens, right?

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Andreas Schwab <schwab@suse.de> writes:

> Alain Schneble <a.s@realize.ch> writes:
>
>> True, but my assumption was that it would be illegal to call
>> `accept-process-output' say, for example, from within
>> `set-process-coding-system' for the sake of waiting for the DNS resolve
>> to complete as it might have unwanted side effects.
>
> You have to wait for the completion event in some way.

Yeah, these blockers would basically have to be a version of this thing
from `send_process':

 while (! new_function_that_says_the_process_is_ready (proc)) 
  wait_reading_process_output (0, 20 * 1000 * 1000, 0, 0, Qnil, NULL, 0);

I think.  Which is what send_process does on EWOULDBLOCK.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't think we should punish users of those packages and their
> developers by breaking their code.  If they didn't get their act
> together, their code will not enjoy the advantages of asynchronicity,
> but it should still work, IMO.  Teaching by punishment rarely works,
> IME, certainly not in a community such as this one.

I don't think it's punishment to signal an error if you're doing
something that's almost certainly erroneous.  As an example where people
are bitten by overly permissive functions, see the long discussion on
whether assoc should signal errors just like member does (or was it the
other way around?).

Quite the reverse: Pretending that there's not a problem here often
sends the developer down a rabbit hole trying to figure out why this
thing doesn't quite work as it should.

Do you think that

(process-send-string (make-network-process ... :nowait t) "foo")

shouldn't signal an error?  It currently does.  I'm not sure that's the
right decision, but I think I'm leaning towards thinking that that's
more helpful than blocking and pretending nothing's wrong...

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alain Schneble <a.s@realize.ch> writes:
>
>> I think the point here is that with these blocking calls in place, we
>> wouldn't really need a separate :nowait 'immediate, just the existing
>> :nowait t.  And IIUC, this is also what Eli Zaretskii proposes.  With
>> this model, :nowait t really means: I request as much async behavior as
>> possible.  But if code is not arranged in an async fashion, then well,
>> you might not get full async behavior.  It's your fault, but we won't
>> signal an error.
>
> If we can identify all the places where we have to insert these
> blockers, then having :nowait t be as async as possible, then that would
> be nice.  I kinda feel that might not be realistic.  But I may be
> wrong.  I mean, there aren't a hundred process-related built-in
> functions...  Your list was just in the tens, right?

I think it should be feasible, really.  The ones I had identified were
these (oh, I think I just looked in process.c, but that should be
sufficient, right?):

`process-contact', `set-process-filter', `set-process-filter-multibyte',
`set-process-window-size', `set-network-process-option',
`set-process-coding-system', `process-filter-multibyte-p',
`process-datagram-address', `set-process-datagram-address',
`process-send-region', `process-send-string', `process-send-eof'

Maybe the last three wouldn't be candidates as they throw an error even
in the current implementation in case the process status is still
'connect, as you and I pointed out already in earlier notes.

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Andreas Schwab <schwab@suse.de> writes:
>
>> Alain Schneble <a.s@realize.ch> writes:
>>
>>> True, but my assumption was that it would be illegal to call
>>> `accept-process-output' say, for example, from within
>>> `set-process-coding-system' for the sake of waiting for the DNS resolve
>>> to complete as it might have unwanted side effects.
>>
>> You have to wait for the completion event in some way.
>
> Yeah, these blockers would basically have to be a version of this thing
> from `send_process':
>
>  while (! new_function_that_says_the_process_is_ready (proc)) 
>   wait_reading_process_output (0, 20 * 1000 * 1000, 0, 0, Qnil, NULL, 0);
>
> I think.  Which is what send_process does on EWOULDBLOCK.

Ok, now I see, thanks to both of you.  In the example above, wait_proc
is NULL.  Shall it be set to the process to wait for in a "real"
implementation?  Together with passing a sigevent struct to
getaddrinfo_a, so the process gets signaled?  Currently, this signaling
is not used, as it polls for completion.  I hope I'm not mixing things
up...

Re: Asynchronous DNS

[Alain Schneble]

Alain Schneble <a.s@realize.ch> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
> Ok, now I see, thanks to both of you.  In the example above, wait_proc
> is NULL.  Shall it be set to the process to wait for in a "real"
> implementation?  Together with passing a sigevent struct to
> getaddrinfo_a, so the process gets signaled?  Currently, this signaling
> is not used, as it polls for completion.  I hope I'm not mixing things
> up...

I'm sorry, please ignore my question.  I think I did mix it up as there
is of course no OS process associated with the network process.  Hence,
the network process can't be signaled and so my question really seems to
be inappropriate...

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> I think it should be feasible, really.  The ones I had identified were
> these (oh, I think I just looked in process.c, but that should be
> sufficient, right?):
>
> `process-contact', `set-process-filter', `set-process-filter-multibyte',
> `set-process-window-size', `set-network-process-option',
> `set-process-coding-system', `process-filter-multibyte-p',
> `process-datagram-address', `set-process-datagram-address',
> `process-send-region', `process-send-string', `process-send-eof'

Yeah, that isn't a very scary-looking list of functions, so perhaps it's
doable without any regressions...

> Maybe the last three wouldn't be candidates as they throw an error even
> in the current implementation in case the process status is still
> 'connect, as you and I pointed out already in earlier notes.

Yup.

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

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: a.s@realize.ch,  emacs-devel@gnu.org
> Date: Tue, 09 Feb 2016 11:47:01 +1100
> 
> Do you think that
> 
> (process-send-string (make-network-process ... :nowait t) "foo")
> 
> shouldn't signal an error?

Yes, I do.

> It currently does.  I'm not sure that's the right decision, but I
> think I'm leaning towards thinking that that's more helpful than
> blocking and pretending nothing's wrong...

I think it's just an accident.  When these APIs were originally
written, computers were sufficiently slow as to have the connection
completed by the time any other API is called.  Computers got much
faster since then, so we will see more and more of these errors.  (I'm
quite sure I see them in smtpmail from time to time, because I use a
very distant SMTP server.)

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Alain Schneble <a.s@realize.ch>
> CC: Eli Zaretskii <eliz@gnu.org>, <emacs-devel@gnu.org>
> Date: Tue, 9 Feb 2016 10:02:33 +0100
> 
> `process-contact', `set-process-filter', `set-process-filter-multibyte',
> `set-process-window-size', `set-network-process-option',
> `set-process-coding-system', `process-filter-multibyte-p',
> `process-datagram-address', `set-process-datagram-address',
> `process-send-region', `process-send-string', `process-send-eof'
> 
> Maybe the last three wouldn't be candidates as they throw an error even
> in the current implementation in case the process status is still
> 'connect, as you and I pointed out already in earlier notes.

But if the wait is for the 'run' status, then it will fix those
problems as well (as I think we should).

Re: Asynchronous DNS

[Alain Schneble]

Eli Zaretskii <eliz@gnu.org> writes:

>> `process-send-region', `process-send-string', `process-send-eof'
>> 
>> Maybe the last three wouldn't be candidates as they throw an error even
>> in the current implementation in case the process status is still
>> 'connect, as you and I pointed out already in earlier notes.
>
> But if the wait is for the 'run' status, then it will fix those
> problems as well (as I think we should).

FWIW, I think I more and more tend to like this approach, where all
those functions would block when called before the socket is connected
-- incl. process-send-* functions that currently signal an error.

Here is a quick (dump 'my 'humble 'mind) of what I think I learned from
all of you so far in this thread and the arguments that make me feel
more and more confident about this promising path:

- We would get a very robust API that will DT(R)T, regardless of whether
  an async expert or a non-expert writes code against in.  Both may fall
  into the blocking "trap".  But those that care will certainly notice
  it.  They will be the ones that read the documentation carefully,
  where it will cover this new async subtlety -- that functions
  accepting an async network process (made with :nowait t) and called
  before its state turned to "open" may block.
  
- It will fix the inconsistency between process-send-* function and the
  others (such as set-process-coding-system), where the former today may
  signal errors while the latter do not.  Having process-send-*
  functions block will certainly only fix issues and shall not introduce
  new ones.

- Current (Lisp) API won't change at all.  No need to extend :nowait.
  Current semantics of nil and t remain.  We do not introduce any new
  idioms or configurations (e.g. that would be the case with 'immediate)
  that we might regret in the future.  It should be fully backward
  compatible.

- As it does not introduce any new concepts, it leaves the path open for
  extensions, should it turn out that a strict async programming model
  might be preferable over "the lax one" described above.  We could
  still introduce things like this:

  (make-network-process ... :nowait 'strict-async
    :sentinel
    (lambda (p s) (
      (await p)
      ...
     )))

  ...where await /must/ be called in this 'strict-async mode.  If p is
  used in a function (e.g. process-send-*) without having awaited it,
  all these functions would signal an error.  So before any usage, one
  must await for p.  If p's state is "open", then it's a no-op.  If not,
  it would block until it is.

Did I miss something?

As said, besides the last point, this really is a summary of other's
(Eli, Lars, Andreas) ideas and remarks.  Thanks you, it's so interesting
:)

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Alain Schneble <a.s@realize.ch>
> Date: Tue, 9 Feb 2016 21:43:10 +0100
> Cc: larsi@gnus.org, emacs-devel@gnu.org
> 
> - We would get a very robust API that will DT(R)T, regardless of whether
>   an async expert or a non-expert writes code against in.  Both may fall
>   into the blocking "trap".  But those that care will certainly notice
>   it.  They will be the ones that read the documentation carefully,
>   where it will cover this new async subtlety -- that functions
>   accepting an async network process (made with :nowait t) and called
>   before its state turned to "open" may block.
>   
> - It will fix the inconsistency between process-send-* function and the
>   others (such as set-process-coding-system), where the former today may
>   signal errors while the latter do not.  Having process-send-*
>   functions block will certainly only fix issues and shall not introduce
>   new ones.
> 
> - Current (Lisp) API won't change at all.  No need to extend :nowait.
>   Current semantics of nil and t remain.  We do not introduce any new
>   idioms or configurations (e.g. that would be the case with 'immediate)
>   that we might regret in the future.  It should be fully backward
>   compatible.
> 
> - As it does not introduce any new concepts, it leaves the path open for
>   extensions, should it turn out that a strict async programming model
>   might be preferable over "the lax one" described above.  We could
>   still introduce things like this:
> 
>   (make-network-process ... :nowait 'strict-async
>     :sentinel
>     (lambda (p s) (
>       (await p)
>       ...
>      )))
> 
>   ...where await /must/ be called in this 'strict-async mode.  If p is
>   used in a function (e.g. process-send-*) without having awaited it,
>   all these functions would signal an error.  So before any usage, one
>   must await for p.  If p's state is "open", then it's a no-op.  If not,
>   it would block until it is.
> 
> Did I miss something?
> 
> As said, besides the last point, this really is a summary of other's
> (Eli, Lars, Andreas) ideas and remarks.  Thanks you, it's so interesting
> :)

Thanks for a nice summary.  I don't think you missed anything.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> - Current (Lisp) API won't change at all.  No need to extend :nowait.
>   Current semantics of nil and t remain.  We do not introduce any new
>   idioms or configurations (e.g. that would be the case with 'immediate)
>   that we might regret in the future.  It should be fully backward
>   compatible.

Well, process-send on a newly made make-network-connection :nowait t
will no longer fail, which is an API change.  :-)

> Did I miss something?

The only thing missing is whether to have a new state for TLS
connections.  When you've asked for a TLS connection, all these commands
should block until TLS has been negotiated, too.  We can have the TLS
commands check for that explicitly (which makes the blocking :nowait
users happy), but we also need a way to tell the non-blocking :nowait
users that the connection is now ready for non-blocking usage.

We could just leave the connection in 'connecting state, perhaps...

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alain Schneble <a.s@realize.ch> writes:
>
>> - Current (Lisp) API won't change at all.  No need to extend :nowait.
>>   Current semantics of nil and t remain.  We do not introduce any new
>>   idioms or configurations (e.g. that would be the case with 'immediate)
>>   that we might regret in the future.  It should be fully backward
>>   compatible.
>
> Well, process-send on a newly made make-network-connection :nowait t
> will no longer fail, which is an API change.  :-)

I categorized this as a minor non-breaking bugfix, for the good, not the
bad.  At least, that was my intention.  :-)

>> Did I miss something?
>
> The only thing missing is whether to have a new state for TLS
> connections.  When you've asked for a TLS connection, all these commands
> should block until TLS has been negotiated, too.  We can have the TLS
> commands check for that explicitly (which makes the blocking :nowait
> users happy), but we also need a way to tell the non-blocking :nowait
> users that the connection is now ready for non-blocking usage.
>
> We could just leave the connection in 'connecting state, perhaps...

FWIW, that sounds very reasonable from my bird's eye view.  I really
kind of ignored the HAVE_GNUTLS paths until now -- which probably was a
mistake...  I'll study them later today.  A solution where client code
is not exposed to a new process state would be the preferred one, I
think.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

By the way, the "make all process functions wait until the connection
has been set up" will make functions that currently are non-blocking
become blocking.  For instance, erc creates a :nowait t connection, and
then calls set-process-coding-system immediately.  And then it sets up a
sentinel that will complete login once the connection changes to 'open.

Unless I'm misreading the code, making functions like that blocking will
make stuff that's currently quite asynchronous become more asynchronous.

We could fix that, of course -- there's really no reason for that
function to be checking infd/outfd, I think?  But this is something that
has to be done on a function by function basis unless we want
regressions in how asynchronous :nowait t connections are.

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> By the way, the "make all process functions wait until the connection
> has been set up" will make functions that currently are non-blocking
> become blocking.  For instance, erc creates a :nowait t connection, and
> then calls set-process-coding-system immediately.  And then it sets up a
> sentinel that will complete login once the connection changes to 'open.
>
> Unless I'm misreading the code, making functions like that blocking will
> make stuff that's currently quite asynchronous become more asynchronous.

You mean less asynchronous. Or more synchronous.  :)  You are right, I
also think it would if the function waits for full connection set up.

> We could fix that, of course -- there's really no reason for that
> function to be checking infd/outfd, I think?  But this is something that
> has to be done on a function by function basis unless we want
> regressions in how asynchronous :nowait t connections are.

Currently, IINW, `set-process-coding-system' calls
`setup_process_coding_systems' which basically requires infd/outfd to
/complete/ its "task".  Otherwise it just returns.  Later in the flow,
`connect_network_socket' will be called and it invokes
`set_network_socket_coding_system' near the end which in turn invokes
`setup_process_coding_systems'.  So it looks like it will complete the
"task" anyway.  But wait, AFAICS, in this case, the encodings that were
specified in the initial call to `set-process-coding-system' will be
overwritten, right?  Hence, I don't think it will work properly if we
just do not check for infd/outfd...

Another approach could be to just wait until the socket has been
initialized (instead of connected), i.e. wait on a more fine grained
(internal) process state than just wait for 'run.  Not sure, but async
TLS handling could benefit from such a state as well.  Of course, we
kind of have it already implicitly, e.g. with p->dns_requests for
example.

As you said, after all, we'll have to look at each case in detail.

Re: Asynchronous DNS

[Andreas Schwab]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> We could fix that, of course -- there's really no reason for that
> function to be checking infd/outfd, I think?

None of the other set-process-* functions have that check so it surely
looks wrong.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Asynchronous DNS

[Alain Schneble]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> We could fix that, of course -- there's really no reason for that
>> function to be checking infd/outfd, I think?
>
> None of the other set-process-* functions have that check so it surely
> looks wrong.

It looks like it's not required in the version on master.  But just
removing the check in the version on the feature/async-dns branch
without accounting for it in any other way seems wrong for the reason
mentioned in my other note before.  Or am I misunderstanding it?

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> Currently, IINW, `set-process-coding-system' calls
> `setup_process_coding_systems' which basically requires infd/outfd to
> /complete/ its "task".  Otherwise it just returns.

Are you thinking about this code?

setup_process_coding_systems (Lisp_Object process)
{
#ifdef subprocesses
  struct Lisp_Process *p = XPROCESS (process);
  int inch = p->infd;
  int outch = p->outfd;
  Lisp_Object coding_system;

  if (inch < 0 || outch < 0)
    return;

With :nowait t now, inch/outch will never be zero here, because we've
crated the sockets, even if we haven't connected them...  So it seems
like it'll complete whatever it's doing even if those sockets aren't
connected.  Possibly.

> Later in the flow, `connect_network_socket' will be called and it
> invokes `set_network_socket_coding_system' near the end which in turn
> invokes `setup_process_coding_systems'.

No, that code is called in make-network-process, so it's done before the
erc.el calls set-process-coding-system, so that's fine.

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alain Schneble <a.s@realize.ch> writes:
>
>> Currently, IINW, `set-process-coding-system' calls
>> `setup_process_coding_systems' which basically requires infd/outfd to
>> /complete/ its "task".  Otherwise it just returns.
>
> Are you thinking about this code?

Yes, I do:

> setup_process_coding_systems (Lisp_Object process)
> {
> #ifdef subprocesses
>   struct Lisp_Process *p = XPROCESS (process);
>   int inch = p->infd;
>   int outch = p->outfd;
>   Lisp_Object coding_system;
>
>   if (inch < 0 || outch < 0)
>     return;
>
> With :nowait t now, inch/outch will never be zero here, because we've
                 ^^^
What do you mean by now?  I was referring to the async DNS resolve case.

> crated the sockets, even if we haven't connected them...  So it seems
> like it'll complete whatever it's doing even if those sockets aren't
> connected.  Possibly.

That's true for the previous non-async DNS code.  But with async DNS,
the socket may not yet have been created.

>> Later in the flow, `connect_network_socket' will be called and it
>> invokes `set_network_socket_coding_system' near the end which in turn
>> invokes `setup_process_coding_systems'.
>
> No, that code is called in make-network-process, so it's done before the
> erc.el calls set-process-coding-system, so that's fine.

Again, true for the code in master, but not for the current code in
feature/async-dns branch, AFAICS.

Re: Asynchronous DNS

[Alain Schneble]

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Lars Ingebrigtsen <larsi@gnus.org>
>>> 
>>> Is there a process list somewhere?
>>
>> See Vprocess_alist.
>
> Thanks.  I don't know how I missed it..  It's even the next-to-last
> statement in make_process, that I thought I had read:
>
>   Vprocess_alist = Fcons (Fcons (name, val), Vprocess_alist);

The following patch removes the separate dns_processes list in favour of
reusing Vprocess_alist.  The latter list is now used to loop over all
processes -- to check if pending DNS requests have completed.

Do you agree whith this change?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch -- Get rid of separate dns_processes list --]
[-- Type: text/x-patch, Size: 4496 bytes --]

From 4967cb5b9f8b9342cae02ae9e2949474606a7fb3 Mon Sep 17 00:00:00 2001
From: Alain Schneble <a.s@realize.ch>
Date: Sun, 14 Feb 2016 00:21:22 +0100
Subject: [PATCH] Get rid of separate dns_processes list

* src/process.c: Remove declaration/definition of dns_processes list.
* src/process.c (wait_reading_process_output): Loop over all processes in
Vprocess_alist instead of dns_processes, to check for completed DNS
requests.
---
 src/process.c | 90 ++++++++++++++++++++++-------------------------------------
 1 file changed, 34 insertions(+), 56 deletions(-)

diff --git a/src/process.c b/src/process.c
index 497b069..ec123e9 100644
--- a/src/process.c
+++ b/src/process.c
@@ -281,10 +281,6 @@ static int max_input_desc;
 
 /* Indexed by descriptor, gives the process (if any) for that descriptor.  */
 static Lisp_Object chan_process[FD_SETSIZE];
-#ifdef HAVE_GETADDRINFO_A
-/* Pending DNS requests. */
-static Lisp_Object dns_processes;
-#endif
 
 /* Alist of elements (NAME . PROCESS).  */
 static Lisp_Object Vprocess_alist;
@@ -3881,7 +3877,6 @@ usage: (make-network-process &rest ARGS)  */)
     {
       p->dns_requests = dns_requests;
       p->status = Qconnect;
-      dns_processes = Fcons (proc, dns_processes);
     }
   else
     {
@@ -4778,51 +4773,41 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 	break;
 
 #ifdef HAVE_GETADDRINFO_A
-      if (!NILP (dns_processes))
-	{
-	  Lisp_Object dns_list = dns_processes, dns, ip_addresses,
-	    answers = Qnil, answer, new = Qnil;
-	  struct Lisp_Process *p;
-
-	  /* This is programmed in a somewhat awkward fashion because
-	  calling connect_network_socket might make us end up back
-	  here again, and we would have a race condition with
-	  segfaults.  So first go through all pending requests and see
-	  whether we got any answers. */
-	  while (!NILP (dns_list))
-	    {
-	      dns = XCAR (dns_list);
-	      dns_list = XCDR (dns_list);
-	      p = XPROCESS (dns);
-	      if (p && p->dns_requests)
-		{
-		  if (! wait_proc || p == wait_proc)
-		    {
-		      ip_addresses = check_for_dns (dns);
-		      if (EQ (ip_addresses, Qt))
-			new = Fcons (dns, new);
-		      else
-			answers = Fcons (Fcons (dns, ip_addresses), answers);
-		    }
-		  else
-		    new = Fcons (dns, new);
-		}
-	    }
-
-	  /* Replace with the list of DNS requests still not responded
-	     to. */
-	  dns_processes = new;
+      {
+	Lisp_Object ip_addresses, answers = Qnil, answer;
+	Lisp_Object process_list_head, async_dns_process_candidate;
+	struct Lisp_Process *p;
+	  
+	/* This is programmed in a somewhat awkward fashion because
+	   calling connect_network_socket might make us end up back
+	   here again, and we would have a race condition with
+	   segfaults.  So first go through all pending requests and see
+	   whether we got any answers. */
+	FOR_EACH_PROCESS(process_list_head, async_dns_process_candidate)
+	  {
+	    p = XPROCESS (async_dns_process_candidate);
+	      
+	    if (p->dns_requests)
+	      {
+		if (! wait_proc || p == wait_proc)
+		  {
+		    ip_addresses = check_for_dns (async_dns_process_candidate);
+		    if (!EQ (ip_addresses, Qt))
+		      answers = Fcons (Fcons (async_dns_process_candidate, ip_addresses), answers);
+		  }
+	      }
+	  }
 
-	  /* Then continue the connection for the successful
-	     requests. */
-	  while (!NILP (answers))
-	    {
-	      answer = XCAR (answers);
-	      answers = XCDR (answers);
-	      if (!NILP (XCDR (answer)))
-		connect_network_socket (XCAR (answer), XCDR (answer));
-	    }
-	}
+	/* Then continue the connection for the successful
+	   requests. */
+	while (!NILP (answers))
+	  {
+	    answer = XCAR (answers);
+	    answers = XCDR (answers);
+	    if (!NILP (XCDR (answer)))
+	      connect_network_socket (XCAR (answer), XCDR (answer));
+	  }
+      }
 #endif /* HAVE_GETADDRINFO_A */
 
       /* Compute time from now till when time limit is up.  */
@@ -7684,9 +7669,6 @@ init_process_emacs (void)
 #ifdef DATAGRAM_SOCKETS
   memset (datagram_address, 0, sizeof datagram_address);
 #endif
-#ifdef HAVE_GETADDRINFO_A
-  dns_processes = Qnil;
-#endif
 
 #if defined (DARWIN_OS)
   /* PTYs are broken on Darwin < 6, but are sometimes useful for interactive
@@ -7774,10 +7756,6 @@ syms_of_process (void)
 
   staticpro (&Vprocess_alist);
   staticpro (&deleted_pid_list);
-#ifdef HAVE_GETADDRINFO_A
-  staticpro (&dns_processes);
-#endif
-
 #endif	/* subprocesses */
 
   DEFSYM (QCname, ":name");
-- 
2.6.2.windows.1

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

>> With :nowait t now, inch/outch will never be zero here, because we've
>                  ^^^
> What do you mean by now?  I was referring to the async DNS resolve case.
>
>> crated the sockets, even if we haven't connected them...  So it seems
>> like it'll complete whatever it's doing even if those sockets aren't
>> connected.  Possibly.
>
> That's true for the previous non-async DNS code.  But with async DNS,
> the socket may not yet have been created.

Yeah, I was talking about the current (and async-dns) :nowait t code.
The :nowait 'immediate code, of course, has other paths.

If we just add blockers to these functions, then code that worked
perfectly well with :nowait t now will stop working (as well).

That's a regression we can't fix for out-of-tree usages of
make-network-process, so I'm back to thinking that we should not change
any of those functions, and :nowait t should work like it does today,
and if you want make-network-process to return before the socket has
been set up (i.e., :nowait 'immediate), then you have to wait until the
process is in state 'run before calling those functions.

I.e., I think we should merge async-dns into trunk as is.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> The following patch removes the separate dns_processes list in favour of
> reusing Vprocess_alist.  The latter list is now used to loop over all
> processes -- to check if pending DNS requests have completed.
>
> Do you agree whith this change?

Well, it changes a test in the network loop that is basically a if (!
NILP (Qnil)) in 99.9999% (plus some nines) of the cases with code that
loops through all processes in the network loop all the time.

That doesn't seem like a good idea, performance wise.

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

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 14 Feb 2016 13:20:21 +1100
> Cc: emacs-devel@gnu.org
> 
> If we just add blockers to these functions, then code that worked
> perfectly well with :nowait t now will stop working (as well).

How would it stop working?  I guess I don't see the problem; can you
describe it?

> That's a regression we can't fix for out-of-tree usages of
> make-network-process, so I'm back to thinking that we should not change
> any of those functions, and :nowait t should work like it does today,
> and if you want make-network-process to return before the socket has
> been set up (i.e., :nowait 'immediate), then you have to wait until the
> process is in state 'run before calling those functions.

Yes.  But AFAIU the suggestion to "add blockers" simply makes these
functions wait until the process is in that state.  So code that did
this outside of those functions will "just work" because these
blockers will become no-ops.

> I.e., I think we should merge async-dns into trunk as is.

No, I don't think so.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes.  But AFAIU the suggestion to "add blockers" simply makes these
> functions wait until the process is in that state.  So code that did
> this outside of those functions will "just work" because these
> blockers will become no-ops.

The issue is losing asynchronicity.  A function that previously called

(progn
  (make-network-stream ... :nowait t)
  (set-process-coding-system ...))

would not block.  With the proposed blockers (unless we add fine-grained
code to all the process function to only block if we haven't even done
DNS yet), this code will block.  And that's a regression.

But, I mean, we could examine all these process functions and only have
them block where we need them to, and things would work fine.  I don't
really have much confidence in us being able to do so completely
transparently, though.

Anyway, my brain may just be in a Gnus clean-up haze here...  that stuff
is tiring work.  But I'm done!  Whee!  I'll start looking at this async
stuff again tomorrow, I think.

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alain Schneble <a.s@realize.ch> writes:
>
>> The following patch removes the separate dns_processes list in favour of
>> reusing Vprocess_alist.  The latter list is now used to loop over all
>> processes -- to check if pending DNS requests have completed.
>>
>> Do you agree whith this change?
>
> Well, it changes a test in the network loop that is basically a if (!
> NILP (Qnil)) in 99.9999% (plus some nines) of the cases with code that
> loops through all processes in the network loop all the time.

True.  Seems like I was too naive. I thought that these additional
cycles were negligible (even though it's O(n)...).

> That doesn't seem like a good idea, performance wise.

Alright.  Anyway, many thanks for looking into it!

Re: Asynchronous DNS

[Stefan Monnier]

> (progn
>   (make-network-stream ... :nowait t)
>   (set-process-coding-system ...))
> would not block.

Why would we have to block?
Can't set-process-coding-system just set some process fields (to
remember which coding-system to use) and return, and then use those
fields later on when the DNS resolution is done?


        Stefan

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: a.s@realize.ch,  emacs-devel@gnu.org
> Date: Sun, 14 Feb 2016 18:01:39 +1100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Yes.  But AFAIU the suggestion to "add blockers" simply makes these
> > functions wait until the process is in that state.  So code that did
> > this outside of those functions will "just work" because these
> > blockers will become no-ops.
> 
> The issue is losing asynchronicity.  A function that previously called
> 
> (progn
>   (make-network-stream ... :nowait t)
>   (set-process-coding-system ...))
> 
> would not block.  With the proposed blockers (unless we add fine-grained
> code to all the process function to only block if we haven't even done
> DNS yet), this code will block.  And that's a regression.

If set-process-coding-system worked in the above snippet, it means it
doesn't need to block waiting for a fully capable process object.  If
it didn't work (as in signaled an error), then there's no regression.

> But, I mean, we could examine all these process functions and only have
> them block where we need them to, and things would work fine.

If the above snippet works now, you have already examined that
function, and you know what the conclusions are.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Alain Schneble <a.s@realize.ch>
> CC: Eli Zaretskii <eliz@gnu.org>, <emacs-devel@gnu.org>
> Date: Sun, 14 Feb 2016 12:19:03 +0100
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > Alain Schneble <a.s@realize.ch> writes:
> >
> >> The following patch removes the separate dns_processes list in favour of
> >> reusing Vprocess_alist.  The latter list is now used to loop over all
> >> processes -- to check if pending DNS requests have completed.
> >>
> >> Do you agree whith this change?
> >
> > Well, it changes a test in the network loop that is basically a if (!
> > NILP (Qnil)) in 99.9999% (plus some nines) of the cases with code that
> > loops through all processes in the network loop all the time.
> 
> True.  Seems like I was too naive. I thought that these additional
> cycles were negligible (even though it's O(n)...).

We could traverse the list when Emacs is idle and maintain the result
in a cache variable.

Re: Asynchronous DNS

[Alain Schneble]

[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> The issue is losing asynchronicity.  A function that previously called
>
> (progn
>   (make-network-stream ... :nowait t)
>   (set-process-coding-system ...))
>
> would not block.  With the proposed blockers (unless we add fine-grained
> code to all the process function to only block if we haven't even done
> DNS yet), this code will block.  And that's a regression.

By the way, I tried it out with the fine-grained approach.  See patch
below.  The majority of the functions touched will block if DNS requests
are still pending.  process-send-* functions block while process is in
"connect" state.  I didn't have time to verify it with TLS connections
though, and also just very quickly with http connections and some hand
crafted scenarios...

> But, I mean, we could examine all these process functions and only have
> them block where we need them to, and things would work fine.  I don't
> really have much confidence in us being able to do so completely
> transparently, though.

There is potential to refactor at least two functions,
set-process-coding-system and set-process-window-size, to not require
blocking at all.  The former will require adaptions in
set_network_socket_coding_system, to check whether coding systems have
already been set, so that they do not get overwritten, e.g. in this
scenario:

(progn
  (make-network-stream ... :nowait t)
  (set-process-coding-system ...))

And set-process-coding-system could just jump over the call to
setup_process_coding_systems in case of a network process with pending
DNS requests (i.e. socket not yet available).

While with set-process-window-size, I asked myself if this one makes any
sense with network processes at all.  Should it signal an error or what
would be the use case to call this for a network process?

And here is the small patch I used:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch -- Make-process-functions-wait-for-DNS-completion --]
[-- Type: text/x-patch, Size: 6439 bytes --]

From 1ceb2e58aa1a7d688439aa9e4c0ed11fb0e50534 Mon Sep 17 00:00:00 2001
From: Alain Schneble <a.s@realize.ch>
Date: Mon, 15 Feb 2016 00:11:06 +0100
Subject: [PATCH] Make process functions wait for DNS completion

* src/process.c (set-process-filter, set-process-window-size,
process-contact, process-datagram-address, set-process-datagram-address,
set-network-process-option): make functions wait (block) on network
process until pending DNS requests have been processed and associated
socket initialized.

* src/process.c (process-send-region, process-send-string,
process-send-eof): make functions wait (block) while network process is
in connect state.
---
 src/process.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 84 insertions(+), 7 deletions(-)

diff --git a/src/process.c b/src/process.c
index 497b069..9816e93 100644
--- a/src/process.c
+++ b/src/process.c
@@ -284,6 +284,7 @@ static Lisp_Object chan_process[FD_SETSIZE];
 #ifdef HAVE_GETADDRINFO_A
 /* Pending DNS requests. */
 static Lisp_Object dns_processes;
+static void wait_for_socket_fds (Lisp_Object process);
 #endif
 
 /* Alist of elements (NAME . PROCESS).  */
@@ -1029,6 +1030,12 @@ The string argument is normally a multibyte string, except:
   struct Lisp_Process *p;
 
   CHECK_PROCESS (process);
+
+#ifdef HAVE_GETADDRINFO_A
+  if (NETCONN_P (process))
+    wait_for_socket_fds (process);
+#endif
+
   p = XPROCESS (process);
 
   /* Don't signal an error if the process's input file descriptor
@@ -1113,6 +1120,11 @@ DEFUN ("set-process-window-size", Fset_process_window_size,
 {
   CHECK_PROCESS (process);
 
+#ifdef HAVE_GETADDRINFO_A
+  if (NETCONN_P (process))
+    wait_for_socket_fds (process);
+#endif
+
   /* All known platforms store window sizes as 'unsigned short'.  */
   CHECK_RANGED_INTEGER (height, 0, USHRT_MAX);
   CHECK_RANGED_INTEGER (width, 0, USHRT_MAX);
@@ -1194,6 +1206,12 @@ list of keywords.  */)
   contact = XPROCESS (process)->childp;
 
 #ifdef DATAGRAM_SOCKETS
+
+#ifdef HAVE_GETADDRINFO_A
+  if (NETCONN_P (process))
+    wait_for_socket_fds (process);
+#endif
+
   if (DATAGRAM_CONN_P (process)
       && (EQ (key, Qt) || EQ (key, QCremote)))
     contact = Fplist_put (contact, QCremote,
@@ -2423,6 +2441,11 @@ DEFUN ("process-datagram-address", Fprocess_datagram_address, Sprocess_datagram_
 
   CHECK_PROCESS (process);
 
+#ifdef HAVE_GETADDRINFO_A
+  if (NETCONN_P (process))
+    wait_for_socket_fds (process);
+#endif
+
   if (!DATAGRAM_CONN_P (process))
     return Qnil;
 
@@ -2442,6 +2465,11 @@ Returns nil upon error setting address, ADDRESS otherwise.  */)
 
   CHECK_PROCESS (process);
 
+#ifdef HAVE_GETADDRINFO_A
+  if (NETCONN_P (process))
+    wait_for_socket_fds (process);
+#endif
+
   if (!DATAGRAM_CONN_P (process))
     return Qnil;
 
@@ -2610,6 +2638,10 @@ OPTION is not a supported option, return nil instead; otherwise return t.  */)
   if (!NETCONN1_P (p))
     error ("Process is not a network process");
 
+#ifdef HAVE_GETADDRINFO_A
+  wait_for_socket_fds (process);
+#endif
+
   s = p->infd;
   if (s < 0)
     error ("Process is not running");
@@ -3693,7 +3725,7 @@ usage: (make-network-process &rest ARGS)  */)
 #endif
 
 #ifdef HAVE_GETADDRINFO_A
-  if (EQ (Fplist_get (contact, QCnowait), Qdns) &&
+  if (EQ (Fplist_get (contact, QCnowait), Qt) &&
       !NILP (host))
     {
       int ret;
@@ -4650,6 +4682,24 @@ check_for_dns (Lisp_Object proc)
 
   return ip_addresses;
 }
+
+static void
+wait_for_socket_fds(Lisp_Object process)
+{
+  while (XPROCESS(process)->dns_requests)
+    {
+      wait_reading_process_output (0, 20 * 1000 * 1000, 0, 0, Qnil, NULL, 0);
+    }
+}
+
+static void
+wait_while_connecting(Lisp_Object process)
+{
+  while (EQ (Qconnect, XPROCESS(process)->status))
+    {
+      wait_reading_process_output (0, 20 * 1000 * 1000, 0, 0, Qnil, NULL, 0);
+    }
+}
 #endif /* HAVE_GETADDRINFO_A */
 
 /* This variable is different from waiting_for_input in keyboard.c.
@@ -6143,6 +6193,11 @@ Output from processes can arrive in between bunches.  */)
   if (XINT (start) < GPT && XINT (end) > GPT)
     move_gap_both (XINT (start), start_byte);
 
+#ifdef HAVE_GETADDRINFO_A
+  if (NETCONN_P (proc))
+    wait_while_connecting (proc);
+#endif
+  
   send_process (proc, (char *) BYTE_POS_ADDR (start_byte),
 		end_byte - start_byte, Fcurrent_buffer ());
 
@@ -6162,6 +6217,12 @@ Output from processes can arrive in between bunches.  */)
   Lisp_Object proc;
   CHECK_STRING (string);
   proc = get_process (process);
+
+#ifdef HAVE_GETADDRINFO_A
+  if (NETCONN_P (proc))
+    wait_while_connecting (proc);
+#endif
+
   send_process (proc, SSDATA (string),
 		SBYTES (string), string);
   return Qnil;
@@ -6576,10 +6637,17 @@ process has been transmitted to the serial port.  */)
   struct coding_system *coding = NULL;
   int outfd;
 
-  if (DATAGRAM_CONN_P (process))
+  proc = get_process (process);
+
+#ifdef HAVE_GETADDRINFO_A
+  if (NETCONN_P (proc))
+    wait_while_connecting (proc);
+#endif
+
+  if (DATAGRAM_CONN_P (proc))
     return process;
 
-  proc = get_process (process);
+  
   outfd = XPROCESS (proc)->outfd;
   if (outfd >= 0)
     coding = proc_encode_coding_system[outfd];
@@ -7030,7 +7098,14 @@ encode subprocess input.  */)
   register struct Lisp_Process *p;
 
   CHECK_PROCESS (process);
+
+#ifdef HAVE_GETADDRINFO_A
+  if (NETCONN_P (process))
+    wait_for_socket_fds (process);
+#endif
+
   p = XPROCESS (process);
+  
   if (p->infd < 0)
     error ("Input file descriptor of %s closed", SDATA (p->name));
   if (p->outfd < 0)
@@ -7067,6 +7142,12 @@ suppressed.  */)
   register struct Lisp_Process *p;
 
   CHECK_PROCESS (process);
+
+#ifdef HAVE_GETADDRINFO_A
+  if (NETCONN_P (process))
+    wait_for_socket_fds (process);
+#endif
+
   p = XPROCESS (process);
   if (NILP (flag))
     pset_decode_coding_system
@@ -7757,7 +7838,6 @@ syms_of_process (void)
   DEFSYM (QCcoding, ":coding");
   DEFSYM (QCserver, ":server");
   DEFSYM (QCnowait, ":nowait");
-  DEFSYM (Qdns, "dns");
   DEFSYM (QCsentinel, ":sentinel");
   DEFSYM (QCtls_parameters, ":tls-parameters");
   DEFSYM (QClog, ":log");
@@ -7921,9 +8001,6 @@ The variable takes effect when `start-process' is called.  */);
 
 #ifdef NON_BLOCKING_CONNECT
    ADD_SUBFEATURE (QCnowait, Qt);
-#ifdef HAVE_GETADDRINFO_A
-   ADD_SUBFEATURE (QCnowait, Qdns);
-#endif
 #endif
 #ifdef DATAGRAM_SOCKETS
    ADD_SUBFEATURE (QCtype, Qdatagram);
-- 
2.6.2.windows.1

Re: Asynchronous DNS

[Alain Schneble]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: a.s@realize.ch,  emacs-devel@gnu.org
>> Date: Sun, 14 Feb 2016 18:01:39 +1100
>> 
>> The issue is losing asynchronicity.  A function that previously called
>> 
>> (progn
>>   (make-network-stream ... :nowait t)
>>   (set-process-coding-system ...))
>> 
>> would not block.  With the proposed blockers (unless we add fine-grained
>> code to all the process function to only block if we haven't even done
>> DNS yet), this code will block.  And that's a regression.
>
> If set-process-coding-system worked in the above snippet, it means it
> doesn't need to block waiting for a fully capable process object.  If
> it didn't work (as in signaled an error), then there's no regression.

set-process-coding-system, how it is currently implemented on master,
requires valid infd/outfd, hence an initialized socket.
make-network-process until now did initialize a socket also with :nowait
t.  That is no longer the case in the feature/async-dns branch with the
:nowait 'dns option, where make-network-stream returns without having
initialized a socket.  In the
"Make-process-functions-wait-for-DNS-completion" patch I sent in the
previous message, set-process-coding-system now waits until the socket
has been initialized.  Thus, there should no longer be any regressions
in this case.

Re: Asynchronous DNS

[Alain Schneble]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> (progn
>>   (make-network-stream ... :nowait t)
>>   (set-process-coding-system ...))
>> would not block.
>
> Why would we have to block?
> Can't set-process-coding-system just set some process fields (to
> remember which coding-system to use) and return, and then use those
> fields later on when the DNS resolution is done?

Yes, that would be a welcome optimization and should be doable with a
small refactoring in set-process-coding-system and
set_network_socket_coding_system, AFAICS.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Alain Schneble <a.s@realize.ch>
> CC: Lars Ingebrigtsen <larsi@gnus.org>, <emacs-devel@gnu.org>
> Date: Mon, 15 Feb 2016 01:14:51 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Lars Ingebrigtsen <larsi@gnus.org>
> >> Cc: a.s@realize.ch,  emacs-devel@gnu.org
> >> Date: Sun, 14 Feb 2016 18:01:39 +1100
> >> 
> >> The issue is losing asynchronicity.  A function that previously called
> >> 
> >> (progn
> >>   (make-network-stream ... :nowait t)
> >>   (set-process-coding-system ...))
> >> 
> >> would not block.  With the proposed blockers (unless we add fine-grained
> >> code to all the process function to only block if we haven't even done
> >> DNS yet), this code will block.  And that's a regression.
> >
> > If set-process-coding-system worked in the above snippet, it means it
> > doesn't need to block waiting for a fully capable process object.  If
> > it didn't work (as in signaled an error), then there's no regression.
> 
> set-process-coding-system, how it is currently implemented on master,
> requires valid infd/outfd, hence an initialized socket.
> make-network-process until now did initialize a socket also with :nowait
> t.  That is no longer the case in the feature/async-dns branch with the
> :nowait 'dns option, where make-network-stream returns without having
> initialized a socket.

Yes, I know.  So there's no regression, since the above snippet
doesn't work on the async-dns branch because set-process-coding-system
signals an error.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Why would we have to block?
> Can't set-process-coding-system just set some process fields (to
> remember which coding-system to use) and return, and then use those
> fields later on when the DNS resolution is done?

We could if that were the only function that had this problem.  I'm not
confident we would be able to do this for all the process related
functions in a way that wouldn't break anything.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> >> The issue is losing asynchronicity.  A function that previously called
>> >> 
>> >> (progn
>> >>   (make-network-stream ... :nowait t)
>> >>   (set-process-coding-system ...))

[...]

> Yes, I know.  So there's no regression, since the above snippet
> doesn't work on the async-dns branch because set-process-coding-system
> signals an error.

No, it does not signal an error on the current async-dns branch, because
:nowait t is fully and completely backwards-compatible on that branch
with :nowait t on the trunk.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> By the way, I tried it out with the fine-grained approach.  See patch
> below.

Thanks, I've applied.  I'll be reworking it a bit to have fewer ifdefs
(and add the TLS checks to the two functions that need it).

> There is potential to refactor at least two functions,
> set-process-coding-system and set-process-window-size, to not require
> blocking at all.

Yeah, we can remove the blockers from the functions that turn out not to
"need them" after we've verified that we're not regressing anything.

> While with set-process-window-size, I asked myself if this one makes any
> sense with network processes at all.  Should it signal an error or what
> would be the use case to call this for a network process?

Uhm...  I can't really imagine what it's used for at all.  :-)  Is it to
communicate to, for instance, a shell what the size of the Emacs window
is?

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

With the latest blockers implemented with some added debugging code,

(eww "https://en.wikipedia.org/wiki/Ocelot")

gives this output:

Async DNS for 'en.wikipedia.org'
Waiting for socket from set-process-filter...
Waiting for socket from set-process-filter...
Async DNS for 'upload.wikimedia.org'
Waiting for socket from set-process-filter...
Async DNS for 'upload.wikimedia.org'
Waiting for socket from set-process-filter...
Async DNS for 'upload.wikimedia.org'
Waiting for socket from set-process-filter...
Async DNS for 'upload.wikimedia.org'
Waiting for socket from set-process-filter...
Async DNS for 'upload.wikimedia.org'
Waiting for socket from set-process-filter...
Async DNS for 'upload.wikimedia.org'
Waiting for socket from set-process-filter...
Waiting for socket from set-process-filter...
Async DNS for 'en.wikipedia.org'
Waiting for socket from set-process-filter...
Waiting for socket from set-process-filter...

Does that function really have to wait for a socket?

It has code like

  if (p->infd >= 0)
    {
      if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
        {
          FD_CLR (p->infd, &input_wait_mask);
          FD_CLR (p->infd, &non_keyboard_wait_mask);
        }
      else if (EQ (p->filter, Qt)
               /* Network or serial process not stopped:  */
               && !EQ (p->command, Qt))
        {
          FD_SET (p->infd, &input_wait_mask);
          FD_SET (p->infd, &non_keyboard_wait_mask);
        }
    }

but I don't think that makes much of a difference...  hm...

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Here's the trace from starting erc:

Async DNS for 'us.ircnet.org'
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...
Waiting for socket from set-process-coding-system...

So it works without regressions, really.  Previously it blocked Emacs
user interactions while waiting for DNS, and it's still blocking Emacs
user interactions while waiting for DNS.  So no difference.

(This can, as discussed, be fixed by fixing that function.)

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

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  <emacs-devel@gnu.org>
> Date: Mon, 15 Feb 2016 15:39:30 +1100
> 
> > While with set-process-window-size, I asked myself if this one makes any
> > sense with network processes at all.  Should it signal an error or what
> > would be the use case to call this for a network process?
> 
> Uhm...  I can't really imagine what it's used for at all.  :-)  Is it to
> communicate to, for instance, a shell what the size of the Emacs window
> is?

Yes.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 15 Feb 2016 17:14:35 +1100
> 
> Async DNS for 'en.wikipedia.org'
> Waiting for socket from set-process-filter...
> Waiting for socket from set-process-filter...
> Async DNS for 'upload.wikimedia.org'
> Waiting for socket from set-process-filter...
> Async DNS for 'upload.wikimedia.org'
> Waiting for socket from set-process-filter...
> Async DNS for 'upload.wikimedia.org'
> Waiting for socket from set-process-filter...
> Async DNS for 'upload.wikimedia.org'
> Waiting for socket from set-process-filter...
> Async DNS for 'upload.wikimedia.org'
> Waiting for socket from set-process-filter...
> Async DNS for 'upload.wikimedia.org'
> Waiting for socket from set-process-filter...
> Waiting for socket from set-process-filter...
> Async DNS for 'en.wikipedia.org'
> Waiting for socket from set-process-filter...
> Waiting for socket from set-process-filter...
> 
> Does that function really have to wait for a socket?

It needs the input file descriptor:

>   if (p->infd >= 0)
>     {
>       if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
>         {
>           FD_CLR (p->infd, &input_wait_mask);        <<<<<<<<<<<<<<<<<<
>           FD_CLR (p->infd, &non_keyboard_wait_mask); <<<<<<<<<<<<<<<<<<
>         }
>       else if (EQ (p->filter, Qt)
>                /* Network or serial process not stopped:  */
>                && !EQ (p->command, Qt))
>         {
>           FD_SET (p->infd, &input_wait_mask);
>           FD_SET (p->infd, &non_keyboard_wait_mask);
>         }
>     }
> 
> but I don't think that makes much of a difference...  hm...

This is needed to set up the various masks that report on the socket
being ready to be read.  The file descriptor comes from the socket, so
you cannot set up those masks until you have the socket.  Of course,
until you do have a socket, no filter will ever need to be invoked, so
this part of the filter setup could be deferred until the connection
succeeds.

Re: Asynchronous DNS

[Alain Schneble]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  <emacs-devel@gnu.org>
>> Date: Mon, 15 Feb 2016 15:39:30 +1100
>> 
>> > While with set-process-window-size, I asked myself if this one makes any
>> > sense with network processes at all.  Should it signal an error or what
>> > would be the use case to call this for a network process?
>> 
>> Uhm...  I can't really imagine what it's used for at all.  :-)  Is it to
>> communicate to, for instance, a shell what the size of the Emacs window
>> is?
>
> Yes.

This sounds like set-process-window-size could just return Qnil when
called on a network process.  This would make the blocker in that
function obsolete.

Re: Asynchronous DNS

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> This is needed to set up the various masks that report on the socket
> being ready to be read.

But the setup doesn't have to be done by set-process-filter.  All the
set-process-* functions should really be only setting a field in the
process object, and all other setup should be done as a side effect of
getting the process connected.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Alain Schneble <a.s@realize.ch>
> CC: Lars Ingebrigtsen <larsi@gnus.org>, <emacs-devel@gnu.org>
> Date: Mon, 15 Feb 2016 12:06:52 +0100
> 
> >> Uhm...  I can't really imagine what it's used for at all.  :-)  Is it to
> >> communicate to, for instance, a shell what the size of the Emacs window
> >> is?
> >
> > Yes.
> 
> This sounds like set-process-window-size could just return Qnil when
> called on a network process.  This would make the blocker in that
> function obsolete.

If the ioctl call issued by the implementation always fails on
sockets, then yes.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  emacs-devel@gnu.org
> Date: Mon, 15 Feb 2016 13:01:17 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > This is needed to set up the various masks that report on the socket
> > being ready to be read.
> 
> But the setup doesn't have to be done by set-process-filter.

That's what I said in the part you elided.

Re: Asynchronous DNS

[Alain Schneble]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alain Schneble <a.s@realize.ch>
>> CC: Lars Ingebrigtsen <larsi@gnus.org>, <emacs-devel@gnu.org>
>> Date: Mon, 15 Feb 2016 12:06:52 +0100
>> 
>> >> Uhm...  I can't really imagine what it's used for at all.  :-)  Is it to
>> >> communicate to, for instance, a shell what the size of the Emacs window
>> >> is?
>> >
>> > Yes.
>> 
>> This sounds like set-process-window-size could just return Qnil when
>> called on a network process.  This would make the blocker in that
>> function obsolete.
>
> If the ioctl call issued by the implementation always fails on
> sockets, then yes.

On GNU/Linux (debian) the call to ioctl (fd, TIOCSWINSZ, &size) in
set_window_size returns -1 if called with a socket fd and thus fails
with errno=25 "Inappropriate ioctl for device".  I guess this might be
the case on other systems as well...

Re: Asynchronous DNS

[Alain Schneble]

[-- Attachment #1: Type: text/plain, Size: 679 bytes --]

Alain Schneble <a.s@realize.ch> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> If the ioctl call issued by the implementation always fails on
>> sockets, then yes.
>
> On GNU/Linux (debian) the call to ioctl (fd, TIOCSWINSZ, &size) in
> set_window_size returns -1 if called with a socket fd and thus fails
> with errno=25 "Inappropriate ioctl for device".  I guess this might be
> the case on other systems as well...

If we will all agree, here is a patch for the feature/async-dns branch
to basically turn set-process-window-size into a "no-op" when called on
network processes.  And it simply makes the call to the "blocker"
function obsolete as a welcome side effect.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch --]
[-- Type: text/x-patch, Size: 1255 bytes --]

From 0d46caf4c7de7bdce3c82e9f620762dea43fd062 Mon Sep 17 00:00:00 2001
From: Alain Schneble <a.s@realize.ch>
Date: Mon, 15 Feb 2016 17:03:38 +0100
Subject: [PATCH] Turn set-process-window-size into a "no-op" for network
 processes

* src/process.c (set-process-window-size): Explicitly return Qnil when
called with network processes as set_window_size won't work anyway on
socket fds.  As a welcome side effect, this makes the blocking
wait_for_socket_fds call obsolete.
---
 src/process.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/process.c b/src/process.c
index f1c066f..f35c88e 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1115,14 +1115,11 @@ DEFUN ("set-process-window-size", Fset_process_window_size,
 {
   CHECK_PROCESS (process);
 
-  if (NETCONN_P (process))
-    wait_for_socket_fds (process, "set-process-window-size");
-
   /* All known platforms store window sizes as 'unsigned short'.  */
   CHECK_RANGED_INTEGER (height, 0, USHRT_MAX);
   CHECK_RANGED_INTEGER (width, 0, USHRT_MAX);
 
-  if (XPROCESS (process)->infd < 0
+  if (NETCONN_P (process) || XPROCESS (process)->infd < 0
       || (set_window_size (XPROCESS (process)->infd,
 			   XINT (height), XINT (width))
 	  < 0))
-- 
2.6.2.windows.1

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alain Schneble <a.s@realize.ch> writes:
>
>> By the way, I tried it out with the fine-grained approach.  See patch
>> below.
>
> Thanks, I've applied.  I'll be reworking it a bit to have fewer ifdefs
> (and add the TLS checks to the two functions that need it).

Thanks for the inclusion and cleanup.

>> There is potential to refactor at least two functions,
>> set-process-coding-system and set-process-window-size, to not require
>> blocking at all.
>
> Yeah, we can remove the blockers from the functions that turn out not to
> "need them" after we've verified that we're not regressing anything.

Yes, that's the approach I had in my mind as well :)

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> +	FOR_EACH_PROCESS(process_list_head, async_dns_process_candidate)
> +	  {
> +	    p = XPROCESS (async_dns_process_candidate);

Actually, thinking a bit more about doing non-blocking TLS negotiation,
doing it this way is probably the best way anyway.  Probably.  When I
start implementing non-blocking TLS boot, then I need a way to keep
prodding libgnutls to continue the negotiation.  So doing both these
things from this loop here is probably the most efficient thing, because
we certainly don't want yet another list of processes for the
semi-negotiated processes...

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> * src/process.c (set-process-window-size): Explicitly return Qnil when
> called with network processes as set_window_size won't work anyway on
> socket fds.  As a welcome side effect, this makes the blocking
> wait_for_socket_fds call obsolete.

Thanks; applied.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> This is needed to set up the various masks that report on the socket
> being ready to be read.  The file descriptor comes from the socket, so
> you cannot set up those masks until you have the socket.  Of course,
> until you do have a socket, no filter will ever need to be invoked, so
> this part of the filter setup could be deferred until the connection
> succeeds.

Yup.  I'll try to refactor that function to do that part of the setup
later, after the connection has been established.

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alain Schneble <a.s@realize.ch> writes:
>
>> * src/process.c (set-process-window-size): Explicitly return Qnil when
>> called with network processes as set_window_size won't work anyway on
>> socket fds.  As a welcome side effect, this makes the blocking
>> wait_for_socket_fds call obsolete.
>
> Thanks; applied.

Thank you, Lars.

Re: Asynchronous DNS

[Alain Schneble]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alain Schneble <a.s@realize.ch>
>> CC: Eli Zaretskii <eliz@gnu.org>, <emacs-devel@gnu.org>
>> Date: Sun, 14 Feb 2016 12:19:03 +0100
>> 
>> Lars Ingebrigtsen <larsi@gnus.org> writes:
>> 
>> > Alain Schneble <a.s@realize.ch> writes:
>> >
>> >> The following patch removes the separate dns_processes list in favour of
>> >> reusing Vprocess_alist.  The latter list is now used to loop over all
>> >> processes -- to check if pending DNS requests have completed.
>> >>
>> >> Do you agree whith this change?
>> >
>> > Well, it changes a test in the network loop that is basically a if (!
>> > NILP (Qnil)) in 99.9999% (plus some nines) of the cases with code that
>> > loops through all processes in the network loop all the time.
>> 
>> True.  Seems like I was too naive. I thought that these additional
>> cycles were negligible (even though it's O(n)...).
>
> We could traverse the list when Emacs is idle and maintain the result
> in a cache variable.

What exactly do you mean by "when Emacs is idle"?  The loop is in
wait_reading_process_output and I thought that this is "kind of"
executed when Emacs is idle.  Or do you refer to idle timers?  I tried
to analyze the call graph starting from command_loop.  I must be missing
something...  Is there yet another concept of "idleness" in Emacs?

Re: Asynchronous DNS

[Lars Ingebrigtsen]

I had assumed that getaddrinfo_a would have the normal timeouts that
getaddrinfo has -- that it would say that it's failed after a certain
number of seconds.  Some testing here (switching off the wifi after
issuing a DNS request) seems to indicate that that may not be the case?

I couldn't find anything in the documentation about this...  am I
overlooking something?  If not, I have to implement some timeouts myself
in that code.

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

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Alain Schneble <a.s@realize.ch>
> CC: <larsi@gnus.org>, <emacs-devel@gnu.org>
> Date: Tue, 16 Feb 2016 22:37:45 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Alain Schneble <a.s@realize.ch>
> >> CC: Eli Zaretskii <eliz@gnu.org>, <emacs-devel@gnu.org>
> >> Date: Sun, 14 Feb 2016 12:19:03 +0100
> >> 
> >> Lars Ingebrigtsen <larsi@gnus.org> writes:
> >> 
> >> > Alain Schneble <a.s@realize.ch> writes:
> >> >
> >> >> The following patch removes the separate dns_processes list in favour of
> >> >> reusing Vprocess_alist.  The latter list is now used to loop over all
> >> >> processes -- to check if pending DNS requests have completed.
> >> >>
> >> >> Do you agree whith this change?
> >> >
> >> > Well, it changes a test in the network loop that is basically a if (!
> >> > NILP (Qnil)) in 99.9999% (plus some nines) of the cases with code that
> >> > loops through all processes in the network loop all the time.
> >> 
> >> True.  Seems like I was too naive. I thought that these additional
> >> cycles were negligible (even though it's O(n)...).
> >
> > We could traverse the list when Emacs is idle and maintain the result
> > in a cache variable.
> 
> What exactly do you mean by "when Emacs is idle"?  The loop is in
> wait_reading_process_output and I thought that this is "kind of"
> executed when Emacs is idle.

That's what I meant.

Re: Asynchronous DNS

[Paul Eggert]

Lars Ingebrigtsen wrote:
> I had assumed that getaddrinfo_a would have the normal timeouts that
> getaddrinfo has -- that it would say that it's failed after a certain
> number of seconds.  Some testing here (switching off the wifi after
> issuing a DNS request) seems to indicate that that may not be the case?

My (admittedly limited) understanding is that getaddrinfo_a doesn't timeout, and 
that the normal way to specify timeouts is via gai_suspend.

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I had assumed that getaddrinfo_a would have the normal timeouts that
> getaddrinfo has -- that it would say that it's failed after a certain
> number of seconds.  Some testing here (switching off the wifi after
> issuing a DNS request) seems to indicate that that may not be the case?

I was just looking into the glibc implementation of getaddrinfo_a again.
It invokes getaddrinfo from its worker threads and it seems like it
should not behave differently regarding timeouts.  The only timeout I
found was to make the worker threads sleep when there is nothing to do.
But they should get woken up if a new request arrives or they will
eventually die when the timeout has elapsed.  But all this should not be
related to the timeout you are referring to.

> I couldn't find anything in the documentation about this...  am I
> overlooking something?  If not, I have to implement some timeouts myself
> in that code.

But what seems strange to me right now is this code in check_for_dns:

  /* The DNS lookup failed. */
  else if (!EQ (p->status, Qconnect))
    {
      ...

Actually, I tried it out and DNS lookups seem to timeout quite fast on
my machine if there is no network connection.  But they do not enter the
if-branch above.  That seems like an error.  Am I day-dreaming or
shouldn't it be rewritten to look like this?

  /* The DNS lookup failed. */
  else if (EQ (p->status, Qconnect))
    {
      ...

Re: Asynchronous DNS

[Alain Schneble]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

Alain Schneble <a.s@realize.ch> writes:

> But what seems strange to me right now is this code in check_for_dns:
>
>   /* The DNS lookup failed. */
>   else if (!EQ (p->status, Qconnect))
>     {
>       ...
>
> Actually, I tried it out and DNS lookups seem to timeout quite fast on
> my machine if there is no network connection.  But they do not enter the
> if-branch above.  That seems like an error.  Am I day-dreaming or
> shouldn't it be rewritten to look like this?
>
>   /* The DNS lookup failed. */
>   else if (EQ (p->status, Qconnect))
>     {
>       ...

And here is the patch.  It indeed seems to resolve the issue.  And
another one as well.  Before, if DNS lookup of at least one process
failed and Emacs was requested to terminate (C-x C-c,
save-buffers-kill-terminal), it hung.  Because there was still one or
several processes in "connect" state.  It was stuck in
wait_for_socket_fds.  Maybe there are other cases where it still does
not behave properly, also after having applied this patch.  I'll try to
understand the other branches, where the process state won't be touched
even if DNS request failed...


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch --]
[-- Type: text/x-patch, Size: 891 bytes --]

From 05ba5057d1ce2394fdaf8d246523166360d3fe89 Mon Sep 17 00:00:00 2001
From: Alain Schneble <a.s@realize.ch>
Date: Sat, 20 Feb 2016 20:37:45 +0100
Subject: [PATCH] Deactivate process if async DNS request failed (bugfix)

* src/process.c (check_for_dns): If the async DNS request failed and the
associated process is still in "connect" state, deactivate the process
and set status to "failed".
---
 src/process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/process.c b/src/process.c
index a59e418..1f83327 100644
--- a/src/process.c
+++ b/src/process.c
@@ -4706,7 +4706,7 @@ check_for_dns (Lisp_Object proc)
       ip_addresses = Fnreverse (ip_addresses);
     }
   /* The DNS lookup failed. */
-  else if (!EQ (p->status, Qconnect))
+  else if (EQ (p->status, Qconnect))
     {
       deactivate_process (proc);
       pset_status (p, (list2
-- 
2.6.2.windows.1

Re: Asynchronous DNS

[Alain Schneble]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Lars Ingebrigtsen wrote:
>> I had assumed that getaddrinfo_a would have the normal timeouts that
>> getaddrinfo has -- that it would say that it's failed after a certain
>> number of seconds.  Some testing here (switching off the wifi after
>> issuing a DNS request) seems to indicate that that may not be the case?
>
> My (admittedly limited) understanding is that getaddrinfo_a doesn't
> timeout, and that the normal way to specify timeouts is via
> gai_suspend.

Here is what my humble self understood:

Yes, getaddrinfo_a returns immediately, as it just enqueues a new
request to be processed by the worker threads.  gai_suspend would only
be needed if a thread wants to wait for a response for a given amount of
time, by blocking the calling thread.

But the async-dns implementation that Lars provided uses another
approach instead: it polls by calling gai_error (succeed, failed, in
progress, ...)  repeatedly from wait_reading_process_output.  As
getaddrinfo_a, gai_error does not block.

AFAIU, in the case of gai_suspend, the client would have to call
gai_error as well just after gai_suspend, to inspect the status of the
requests that have completed.  But as gai_suspend would block the
calling thread, it wouldn't be a wise choice for async-dns
implementation in GNU Emacs, as it would essentially block it as well.
But after all, IIUC, using gai_suspend is optional.  So it should just
work properly also with the polling approach currently in place.

Re: Asynchronous DNS

[Paul Eggert]

Alain Schneble wrote:
> But the async-dns implementation that Lars provided uses another
> approach instead: it polls by calling gai_error (succeed, failed, in
> progress, ...)  repeatedly from wait_reading_process_output.

You're ahead of me, since I haven't read the implementation. From what you say, 
it appears that Emacs needs to cancel DNS requests that it thinks are so old 
that the answer is not likely to be useful. Presumably this could be done just 
before calling gai_error, by calling gai_cancel on the old requests.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> And here is the patch.  It indeed seems to resolve the issue.

Great, thanks.  Applied.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

I think all of Eli's comments have now been taken care of on the
async-dns branch, and all tests complete successfully, and everything
seems to work fine for me in daily use (Gnus, eww, erc, Emacs Server).

But it would be nice if somebody could do a code review before merging
to master.  Should I just post the diff here?  It's rather large, but
mainly because many humongous functions have been split up into smaller,
but still too-large, functions...

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

Re: Asynchronous DNS

[Alain Schneble]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Alain Schneble wrote:
>> But the async-dns implementation that Lars provided uses another
>> approach instead: it polls by calling gai_error (succeed, failed, in
>> progress, ...)  repeatedly from wait_reading_process_output.
>
> You're ahead of me, since I haven't read the implementation. From what
> you say, it appears that Emacs needs to cancel DNS requests that it
> thinks are so old that the answer is not likely to be
> useful. Presumably this could be done just before calling gai_error,
> by calling gai_cancel on the old requests.

That's a good point.  Calling delete-process on a process with a pending
DNS request is the only situation so far where we explicitly cancel the
pending request using gai_cancel.

But I don't think we have to care about timing out DNS requests on our
own.  That should already be handled by the synchronous getaddrinfo that
gets called in the worker threads of getaddrinfo_a.  It should be safe
to assume that the timeouts that getaddrinfo uses were chosen wisely, I
think.  To me, there seems to be no need to introduce yet another
timeout on "our" level.

...and the behavior with the recent patch looks promising :)

Re: Asynchronous DNS

[Alain Schneble]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I think all of Eli's comments have now been taken care of on the
> async-dns branch, and all tests complete successfully, and everything
> seems to work fine for me in daily use (Gnus, eww, erc, Emacs Server).

Just two minor points that came to my mind right now:

- Shouldn't the printf statements -- that were used for debugging
  purposes -- be removed before merging to master?  See also attached
  patch below.
  
- Would it be worth to mention somewhere, that some of the functions may
  block also for :nowait t when called outside of the sentinel callback
  (and in case the connection has not yet been established)?  Or that
  they in turn should be called within the sentinel to get max
  asynchronous behavior in any case?  For example, we could add a
  sentence to make-network-process in the description of :nowait param.
  What do you think?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch --]
[-- Type: text/x-patch, Size: 1756 bytes --]

From db3fbe5e20c42c1034192d69ee5ccebdcfb57e96 Mon Sep 17 00:00:00 2001
From: Alain Schneble <a.s@realize.ch>
Date: Sun, 21 Feb 2016 12:02:02 +0100
Subject: [PATCH] Remove async DNS printf debug statements

* src/process.c (make-network-process, wait_for_socket_fds,
wait_while_connecting, wait_for_tls_negotiation): Remove printf debug
statements.
---
 src/process.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/process.c b/src/process.c
index 1f83327..6040be0 100644
--- a/src/process.c
+++ b/src/process.c
@@ -3777,7 +3777,6 @@ usage: (make-network-process &rest ARGS)  */)
     {
       int ret;
 
-      printf("Async DNS for '%s'\n", SSDATA (host));
       dns_requests = xmalloc (sizeof (struct gaicb*));
       dns_requests[0] = xmalloc (sizeof (struct gaicb));
       dns_requests[0]->ar_name = strdup (SSDATA (host));
@@ -4733,7 +4732,6 @@ wait_for_socket_fds (Lisp_Object process, char *name)
   while (XPROCESS (process)->infd < 0 &&
 	 EQ (XPROCESS (process)->status, Qconnect))
     {
-      printf("Waiting for socket from %s...\n", name);
       wait_reading_process_output (0, 20 * 1000 * 1000, 0, 0, Qnil, NULL, 0);
     }
 }
@@ -4743,7 +4741,6 @@ wait_while_connecting (Lisp_Object process)
 {
   while (EQ (XPROCESS (process)->status, Qconnect))
     {
-      printf("Waiting for connection...\n");
       wait_reading_process_output (0, 20 * 1000 * 1000, 0, 0, Qnil, NULL, 0);
     }
 }
@@ -4755,7 +4752,6 @@ wait_for_tls_negotiation (Lisp_Object process)
   while (XPROCESS (process)->gnutls_p &&
 	 XPROCESS (process)->gnutls_initstage != GNUTLS_STAGE_READY)
     {
-      printf("Waiting for TLS...\n");
       wait_reading_process_output (0, 20 * 1000 * 1000, 0, 0, Qnil, NULL, 0);
     }
 #endif
-- 
2.6.2.windows.1

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But it would be nice if somebody could do a code review before merging
> to master.  Should I just post the diff here?  It's rather large, but
> mainly because many humongous functions have been split up into smaller,
> but still too-large, functions...

FWIW, I just wanted to say that I will for sure review the changes at
least to enhance my personal knowledge in that area.  But as I do not
yet have any expertise in "GNU Emacs core", this might be of very little
value for you.  So you for sure won't trust me as a reviewer and I think
you shouldn't either, really.  But anyway, I'm trying to get into it for
the future... ;)

BTW, I'll try to make at least set-process-coding-system non-blocking as
an exercise.  If that won't be patched by somebody else in the meantime,
I'll be happy to supply a patch for it in case I really succeed.  But
that could be applied to master after having merged the async-dns branch
back to master.

For me, posting a separate patch of all the changes from the async-dns
branch isn't needed.  It's so easy to follow the changes in the branch
directly.

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Alain Schneble <a.s@realize.ch>
> Date: Sun, 21 Feb 2016 19:55:43 +0100
> 
> - Shouldn't the printf statements -- that were used for debugging
>   purposes -- be removed before merging to master?  See also attached
>   patch below.

Yes, of course.

> - Would it be worth to mention somewhere, that some of the functions may
>   block also for :nowait t when called outside of the sentinel callback
>   (and in case the connection has not yet been established)?

This should be in the doc string.

>                                                              Or that
>   they in turn should be called within the sentinel to get max
>   asynchronous behavior in any case?  For example, we could add a
>   sentence to make-network-process in the description of :nowait param.

This should be in the manual, I think.

Thanks.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> FWIW, I just wanted to say that I will for sure review the changes at
> least to enhance my personal knowledge in that area.  But as I do not
> yet have any expertise in "GNU Emacs core", this might be of very little
> value for you.  So you for sure won't trust me as a reviewer and I think
> you shouldn't either, really.  But anyway, I'm trying to get into it for
> the future... ;)

It has great value for me.  :-)  But more eyes are nice, too.

> BTW, I'll try to make at least set-process-coding-system non-blocking as
> an exercise.  If that won't be patched by somebody else in the meantime,
> I'll be happy to supply a patch for it in case I really succeed.  But
> that could be applied to master after having merged the async-dns branch
> back to master.

Yeah, I think that can be done after merging.  That it blocks currently
is not a regression.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> Just two minor points that came to my mind right now:
>
> - Shouldn't the printf statements -- that were used for debugging
>   purposes -- be removed before merging to master?  See also attached
>   patch below.

I think the "blocker" messages should perhaps be turned into those
"log them to the *Messages* buffer" warnings.  Err...  that's
add_to_log, I guess.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> - Would it be worth to mention somewhere, that some of the functions may
>>   block also for :nowait t when called outside of the sentinel callback
>>   (and in case the connection has not yet been established)?
>
> This should be in the doc string.
>
>>                                                              Or that
>>   they in turn should be called within the sentinel to get max
>>   asynchronous behavior in any case?  For example, we could add a
>>   sentence to make-network-process in the description of :nowait param.
>
> This should be in the manual, I think.

I've now added this.

So...  is it OK if I merge to master now and we can continue to tweak
stuff there?

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

Re: Asynchronous DNS

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 22 Feb 2016 13:15:43 +1100
> Cc: Alain Schneble <a.s@realize.ch>, emacs-devel@gnu.org
> 
> So...  is it OK if I merge to master now and we can continue to tweak
> stuff there?

No objections from me.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> So...  is it OK if I merge to master now and we can continue to tweak
>> stuff there?
>
> No objections from me.

Ok; here goes.  *crosses fingers*

I've tried to ensure Emacs would build with various permutations of
getaddrinfo, getaddrinfo_a, non-blocking socket and TLS support, but I
have almost certainly not gotten them all, so please report any
resulting build errors.

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

And the obligatory video evidence:

http://lars.ingebrigtsen.no/2016/02/22/a-big-patch-for-emacs-a-small-step-for-eww/

:-)

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Amazingly enough, there have been no bug reports from the networking
code hackery, so I'm now continuing to tweak stuff a bit.

I've removed the blockers from the process setting functions, and
deferred completion of the setup to the async code.  It seems to work
fine -- starting erc is now completely non-blocking.

I wonder whether I should perhaps start rewriting the getaddrinfo_a code
to, well, not use getaddrinfo_a, but instead our own function that
starts a thread and just calls getaddrinfo/gethostbyname.  That would
make this stuff work on BSD, Apple and Windows, too, I think?

But I only have a few days left of my holiday, so perhaps that's not
realistic...  Hm...

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

Re: Asynchronous DNS

[Paul Eggert]

Lars Ingebrigtsen wrote:
> I wonder whether I should perhaps start rewriting the getaddrinfo_a code
> to, well, not use getaddrinfo_a, but instead our own function that
> starts a thread and just calls getaddrinfo/gethostbyname.  That would
> make this stuff work on BSD, Apple and Windows, too, I think?

Should work, yes. But shouldn't this be a Gnulib module? That way, the main part 
of the Emacs source code could simply assume that getaddrinfo_a works, and a 
Gnulib substitute would be built on platforms where it doesn't.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Should work, yes. But shouldn't this be a Gnulib module? That way, the
> main part of the Emacs source code could simply assume that
> getaddrinfo_a works, and a Gnulib substitute would be built on
> platforms where it doesn't.

Yes, that would be even better.

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

Re: Asynchronous DNS

[John Wiegley]

>>>>> Lars Ingebrigtsen <larsi@gnus.org> writes:

> Amazingly enough, there have been no bug reports from the networking code
> hackery, so I'm now continuing to tweak stuff a bit.

Well, bear in mind that most people are using emacs-25, and not master
directly, so your candidate pool of users should be somewhat small right now.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Asynchronous DNS

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 21 Feb 2016 11:29:12 +0100, Alain Schneble <a.s@realize.ch> said:

> That's a good point.  Calling delete-process on a process with a
> pending DNS request is the only situation so far where we explicitly
> cancel the pending request using gai_cancel.

   834	DEFUN ("delete-process", Fdelete_process, Sdelete_process, 1, 1, 0,
   835	       doc: /* Delete PROCESS: kill it and forget about it immediately.
   836	PROCESS may be a process, a buffer, the name of a process or buffer, or
   837	nil, indicating the current buffer's process.  */)
   838	  (register Lisp_Object process)
   839	{
   840	  register struct Lisp_Process *p;
   841	
   842	  process = get_process (process);
   843	  p = XPROCESS (process);
   844	
   845	#ifdef HAVE_GETADDRINFO_A
   846	  if (p->dns_request)
   847	    {
   848	      gai_cancel (p->dns_request);
   849	      free_dns_request (process);
   850	    }
   851	#endif

The man page says gai_cancel does not cancel the request if it is
currently being processed.  So I suspect it is unsafe to free struct
gaicb by calling free_dns_request if the request was not canceled
actually.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Asynchronous DNS

[Lars Ingebrigtsen]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

>    845	#ifdef HAVE_GETADDRINFO_A
>    846	  if (p->dns_request)
>    847	    {
>    848	      gai_cancel (p->dns_request);
>    849	      free_dns_request (process);
>    850	    }
>    851	#endif
>
> The man page says gai_cancel does not cancel the request if it is
> currently being processed.  So I suspect it is unsafe to free struct
> gaicb by calling free_dns_request if the request was not canceled
> actually.

Yes, I think you're right.  If we check the status and it's
EAI_CANCELLED, then it's safe to free the request.  However, what do we
do if it isn't safe?  This is in `delete-process', so the process object
itself is going to go away, and then we lose track of p->dns_request, so
we can't free it.  I think.

Anybody got any ideas for scheduling that free_dns_request?

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

Re: Asynchronous DNS

[Alain Schneble]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:
>
>> The man page says gai_cancel does not cancel the request if it is
>> currently being processed.  So I suspect it is unsafe to free struct
>> gaicb by calling free_dns_request if the request was not canceled
>> actually.
>
> Yes, I think you're right.  If we check the status and it's
> EAI_CANCELLED, then it's safe to free the request.  However, what do we
> do if it isn't safe?  This is in `delete-process', so the process object
> itself is going to go away, and then we lose track of p->dns_request, so
> we can't free it.  I think.
>
> Anybody got any ideas for scheduling that free_dns_request?

I see these options:

1. Suspend the thread using gai_suspend, waiting for the DNS request to
   complete.
   
2. Calling a "blocker" similar to wait_for_socket_fds, to wait for DNS
   request completion in a loop.  But this would need additional logic
   in wait_reading_process_output, to not proceed with
   connect_network_socket in case process deletion is pending.

3. Any other way of postponing freeaddrinfo.

I'm not sure, but my gut feeling says that option #1 may be acceptable
in this situation.  We might run into "suspend" only in corner cases, if
the process is being deleted while a request is still being processed.

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Alain Schneble <a.s@realize.ch> writes:

> 1. Suspend the thread using gai_suspend, waiting for the DNS request to
>    complete.
>
> 2. Calling a "blocker" similar to wait_for_socket_fds, to wait for DNS
>    request completion in a loop.  But this would need additional logic
>    in wait_reading_process_output, to not proceed with
>    connect_network_socket in case process deletion is pending.
>
> 3. Any other way of postponing freeaddrinfo.
>
> I'm not sure, but my gut feeling says that option #1 may be acceptable
> in this situation.  We might run into "suspend" only in corner cases, if
> the process is being deleted while a request is still being processed.

Yes, I agree.  Suspending seems like the best solution here, but I can
imagine unfortunate consequences when doing that.

For instance, if Emacs is shutting down, and it just calls
`delete-process' on everything (which it does, I think?), then Emacs
exit would hang until getaddrinfo returns (which may take many seconds).
(That is, if Emacs just has decided to resolve something in the
background, for instance by erc deciding to reconnect or something.)

Could we detect whether Emacs is shutting down, perhaps?

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

Re: Asynchronous DNS

[Andreas Schwab]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> For instance, if Emacs is shutting down, and it just calls
> `delete-process' on everything (which it does, I think?), then Emacs
> exit would hang until getaddrinfo returns (which may take many seconds).

shut_down_emacs sets inhibit_sentinels before calling
kill_buffer_processes, which could be used as an indicator to avoid
waiting.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Asynchronous DNS

[Lars Ingebrigtsen]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> For instance, if Emacs is shutting down, and it just calls
>> `delete-process' on everything (which it does, I think?), then Emacs
>> exit would hang until getaddrinfo returns (which may take many seconds).
>
> shut_down_emacs sets inhibit_sentinels before calling
> kill_buffer_processes, which could be used as an indicator to avoid
> waiting.

Great!  I'll use that, and use gai_suspend if it's not set...

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

Re: Asynchronous DNS

[Lars Ingebrigtsen]

A C question.  I did the obvious

	      gai_suspend (&p->dns_request, 1, NULL);

and that gave me this warning:

process.c: In function 'Fdelete_process':
process.c:861:21: warning: passing argument 1 of 'gai_suspend' from incompatible pointer type [-Wincompatible-pointer-types]
        gai_suspend (&p->dns_request,
                     ^
In file included from process.c:38:0:
/usr/include/netdb.h:703:12: note: expected 'const struct gaicb * const*' but argument is of type 'struct gaicb **'
 extern int gai_suspend (const struct gaicb *const __list[], int __ent,
            ^

Is the only difference here really the consts?  If so, the following
cast should make stuff "work", but...

	      gai_suspend ((const struct gaicb * const*)&p->dns_request,
			   1, NULL);



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

Re: Asynchronous DNS

[Andreas Schwab]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> A C question.  I did the obvious
>
> 	      gai_suspend (&p->dns_request, 1, NULL);
>
> and that gave me this warning:
>
> process.c: In function 'Fdelete_process':
> process.c:861:21: warning: passing argument 1 of 'gai_suspend' from incompatible pointer type [-Wincompatible-pointer-types]
>         gai_suspend (&p->dns_request,
>                      ^
> In file included from process.c:38:0:
> /usr/include/netdb.h:703:12: note: expected 'const struct gaicb * const*' but argument is of type 'struct gaicb **'
>  extern int gai_suspend (const struct gaicb *const __list[], int __ent,
>             ^
>
> Is the only difference here really the consts?  If so, the following
> cast should make stuff "work", but...

Yes, it's a hole in the C type system that has been fixed in C++
(http://c-faq.com/ansi/constmismatch.html).

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."