Missing sentinel events

Missing sentinel events

[Lars Ingebrigtsen]

While looking at all the failure modes, we seem to be lacking a few
during async connection.

The manual says that sentinels get codes like:

-----
The string describing the event looks like one of the following:

    "finished\n".
    "deleted\n".
    "exited abnormally with code exitcode (core dumped)\n". The “core
    dumped” part is optional, and only appears if the process dumped
    core.
    "failed with code fail-code\n".
    [many more removed here]
---

But this is the code from process.c:

  else if (p->outfd < 0)
    {
      /* The counterparty may have closed the connection (especially
	 if the NSM prompt above take a long time), so recheck the file
	 descriptor here. */
      pset_status (p, Qfailed);
      deactivate_process (proc);
    }
  else if ((fd_callback_info[p->outfd].flags & NON_BLOCKING_CONNECT_FD) == 0)
    {
      /* If we cleared the connection wait mask before we did the TLS
	 setup, then we have to say that the process is finally "open"
	 here. */
      pset_status (p, Qrun);
      /* Execute the sentinel here.  If we had relied on status_notify
	 to do it later, it will read input from the process before
	 calling the sentinel.  */
      exec_sentinel (proc, build_string ("open\n"));
    }

I suspect that I just forgot to put those calls to exec_sentinel into
the "fail" bits here and other places in the code when I rewrote these
parts last February, like this:

  /* The DNS lookup failed. */
  else if (connecting_status (p->status))
    {
      deactivate_process (proc);
      pset_status (p, (list2
		       (Qfailed,
			concat3 (build_string ("Name lookup of "),
				 build_string (p->dns_request->ar_name),
				 build_string (" failed")))));
    }

Doesn't it seem like these should be calling the sentinel, too?  I think
so, but there's a slight chance that adding more sentinel calls will
break some user-level code that's not expecting to see any new
messages...

And the manual says "looks like one of the following", and people may
have interpreted that as an exhaustive list of event strings.

Any thoughts?  It's incomplete as it is now, so we should definitely add
more sentinel calls, but should they all look like "failed with code
fail-code\n" or should they be... more meaningful?

I think perhaps I should just add "more meaningful" sentinel messages,
and if it turns out to be a problem in real life, we can change them all
to "failed with code" messages...

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

Re: Missing sentinel events

[Lars Ingebrigtsen]

Hm...  There's a mechanism to send out all process statuses to sentinels?

	  /* Get the text to use for the message.  */
	  if (p->raw_status_new)
	    update_status (p);
	  msg = status_message (p);


[...]

	  /* The actions above may have further incremented p->tick.
	     So set p->update_tick again so that an error in the sentinel will
	     not cause this code to be run again.  */
	  p->update_tick = p->tick;
	  /* Now output the message suitably.  */
	  exec_sentinel (proc, msg);

So...  the code in question should really just be doing

		      p->tick = ++process_tick;

?

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

Re: Missing sentinel events

[Lars Ingebrigtsen]

If I just put a tick++ here and there, my sentinel now gets:

failed with code Name lookup of laars.ingebrigtsen.no failed

Is that...  OK?  :-)

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

Re: Missing sentinel events

[Stefan Monnier]

> If I just put a tick++ here and there, my sentinel now gets:
> failed with code Name lookup of laars.ingebrigtsen.no failed
> Is that...  OK?  :-)

I think that's right, yes.  The "the event looks like one of the
following" wording already implies the list is not exhaustive (and
indeed, I don't think it is), so it's perfectly OK to add more cases.


        Stefan

Re: Missing sentinel events

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sun, 22 Jan 2017 12:52:58 -0500
> 
> The "the event looks like one of the following" wording already
> implies the list is not exhaustive (and indeed, I don't think it is)

It's supposed to be exhaustive.  It was when I wrote that.  If you
find a string that isn't in the list, please add it to the list.

Re: Missing sentinel events

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Date: Sun, 22 Jan 2017 12:52:58 -0500
>> 
>> The "the event looks like one of the following" wording already
>> implies the list is not exhaustive (and indeed, I don't think it is)
>
> It's supposed to be exhaustive.  It was when I wrote that.  If you
> find a string that isn't in the list, please add it to the list.

But do you think "failed with code Name lookup of laars.ingebrigtsen.no
failed" message fits the "failed with code codename" template
sufficiently?  (The two "failed" bits is probably something that should
be tweaked.)

If not, do you have a suggestion as to a better sentiel code message?  I
like that it's so explicit: It really says what the problem was...

The other messages that may appear now are:

failed with code The Network Security Manager stopped the connections
failed with code TLS negotiation failed

And...  I think that's... it?  Probably.  If I skim the code correctly,
these are the only three newly added error conditions, and that aren't
currently triggering the sentinel.

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

Re: Missing sentinel events

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  emacs-devel@gnu.org
> Date: Sun, 22 Jan 2017 19:22:19 +0100
> 
> But do you think "failed with code Name lookup of laars.ingebrigtsen.no
> failed" message fits the "failed with code codename" template
> sufficiently?

Yes, I think so.

> (The two "failed" bits is probably something that should be
> tweaked.)

That's something for the sentinel to worry about.  Emacs core should
only prepare the data.

> The other messages that may appear now are:
> 
> failed with code The Network Security Manager stopped the connections
> failed with code TLS negotiation failed

They still match the "failed with code SOMETHING" form.

> And...  I think that's... it?  Probably.

So the list is still exhaustive.

Re: Missing sentinel events

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 22 Jan 2017 18:09:40 +0100
> 
> The manual says that sentinels get codes like:
> 
> -----
> The string describing the event looks like one of the following:
> 
>     "finished\n".
>     "deleted\n".
>     "exited abnormally with code exitcode (core dumped)\n". The “core
>     dumped” part is optional, and only appears if the process dumped
>     core.
>     "failed with code fail-code\n".
>     [many more removed here]
> ---
> 
> But this is the code from process.c:
> 
>   else if (p->outfd < 0)
>     {
>       /* The counterparty may have closed the connection (especially
> 	 if the NSM prompt above take a long time), so recheck the file
> 	 descriptor here. */
>       pset_status (p, Qfailed);
>       deactivate_process (proc);
>     }
>   else if ((fd_callback_info[p->outfd].flags & NON_BLOCKING_CONNECT_FD) == 0)
>     {
>       /* If we cleared the connection wait mask before we did the TLS
> 	 setup, then we have to say that the process is finally "open"
> 	 here. */
>       pset_status (p, Qrun);
>       /* Execute the sentinel here.  If we had relied on status_notify
> 	 to do it later, it will read input from the process before
> 	 calling the sentinel.  */
>       exec_sentinel (proc, build_string ("open\n"));
>     }
> 
> I suspect that I just forgot to put those calls to exec_sentinel into
> the "fail" bits here and other places in the code when I rewrote these
> parts last February, like this:
> 
>   /* The DNS lookup failed. */
>   else if (connecting_status (p->status))
>     {
>       deactivate_process (proc);
>       pset_status (p, (list2
> 		       (Qfailed,
> 			concat3 (build_string ("Name lookup of "),
> 				 build_string (p->dns_request->ar_name),
> 				 build_string (" failed")))));
>     }
> 
> Doesn't it seem like these should be calling the sentinel, too?  I think
> so, but there's a slight chance that adding more sentinel calls will
> break some user-level code that's not expecting to see any new
> messages...
> 
> And the manual says "looks like one of the following", and people may
> have interpreted that as an exhaustive list of event strings.

The list is exhaustive, so that interpretation is correct.

You are looking at the error symbol that is put into the process
status, but that's not what the sentinel will see.  It will see what
status_message will produce given the symbol.  So you really must look
at status_message and the strings it produces to see whether the list
in the manual is exhaustive or not.

> Any thoughts?  It's incomplete as it is now, so we should definitely add
> more sentinel calls, but should they all look like "failed with code
> fail-code\n" or should they be... more meaningful?

What more meaningful messages did you have in mind?  It's hard to
reason in the abstract.