bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'

bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'

[Dmitry]

Note: this could be considered a regression from Emacs 24.3

With this test case:

```
(require 'url-http)

(defvar uht-counter 0)

(defun uht-callback (status)
  (declare (special url-http-process))
  (message "%s %s" uht-counter status)
  (delete-process url-http-process))

(defun uht-test ()
  (setq uht-counter (1+ uht-counter))
  ;; The port must not be open.
  (url-http (url-generic-parse-url "http://localhost:3333") #'uht-callback (list 'foo)))
```

Evaluate `(uht-test)' and see two messages in the message log with the
same counter value. The second message outputs a different STATUS value,
caused by modification of `url-callback-arguments' in
`url-http-async-sentinel', like this:

4 (:error (error connection-failed failed with code 111
 :host localhost :service 3333) . foo)
4 (:error (error connection-failed deleted
 :host localhost :service 3333) :error (error connection-failed failed with code 111
 :host localhost :service 3333) . foo)

If the callback expects STATUS to contain some specific data structure,
that can cause breakage, see https://github.com/marijnh/tern/issues/350
for an example.

Now, I'm not entirely sure the problem is in `url-http'. It does not
manifest in Emacs 24.3, possibly because in the current pretest, when
the connection fails, the `url-http-process' is still alive when
`delete-process' is called in `uht-callback'. This is not the case in
the current stable Emacs (wherein, as a consequence, `uht-callback'
doesn't get called the second time), so this could be a regression in
the processes subsystem.


In GNU Emacs 24.3.92.3 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8)
 of 2014-07-24 on axl
Repository revision: 117398 stephen.berman@gmx.net-20140722213204-51v7bp0chfei6wbx
Windowing system distributor `The X.Org Foundation', version 11.0.11501000
System Description:	Ubuntu 14.04.1 LTS

bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'

[Glenn Morris]

Maybe you could bisect for the cause?

bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'

[Óscar Fuentes]

Git bisect:

488ac8e4deedd56665301762fe6ad2e9e9dea7e7 is the first bad commit
commit 488ac8e4deedd56665301762fe6ad2e9e9dea7e7
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Wed May 15 14:54:49 2013 -0400

    * src/process.c: Export default filters and sentinels to Elisp.
    (Qinternal_default_process_sentinel, Qinternal_default_process_filter):
    New constants.
    (pset_filter, pset_sentinel, make_process, Fset_process_filter)
    (Fset_process_sentinel, Fformat_network_address):
    Default to them instead of nil.
    (server_accept_connection): Sentinels can't be nil any more.
    (read_and_dispose_of_process_output): New function, extracted from
    read_process_output.
    (read_process_output): Use it; filters can't be nil.
    (Finternal_default_process_filter): New function, extracted from
    read_process_output.
    (exec_sentinel_unwind): Remove function.
    (exec_sentinel): Don't zilch sentinel while running.
    (status_notify): Sentinels can't be nil.
    (Finternal_default_process_sentinel): New function extracted from
    status_notify.
    (setup_process_coding_systems): Default filter is not nil any more.
    (syms_of_process): Export new Elisp functions and initialize
    new constants.
    * src/lisp.h (make_lisp_proc): New function.

bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'

[Stefan Monnier]

> (require 'url-http)
> (defvar uht-counter 0)
> (defun uht-callback (status)
>   (declare (special url-http-process))
>   (message "%s %s" uht-counter status)
>   (delete-process url-http-process))
> (defun uht-test ()
>   (setq uht-counter (1+ uht-counter))
>   ;; The port must not be open.
>   (url-http (url-generic-parse-url "http://localhost:3333") #'uht-callback (list 'foo)))

[ Thanks a lot Óscar for bisecting this one.  ]

Dmitry, could you explain a bit more of the context?

The problem is that calling `delete-process' may run the sentinel (and
since this is code run from the sentinel, it may end up calling the
sentinel recursively).

So if you don't want the sentinel to be called recursively, you'll want
to (set-process-sentinel url-http-process nil) before calling
delete-process (or refrain from calling delete-process).

> If the callback expects STATUS to contain some specific data structure,
> that can cause breakage, see https://github.com/marijnh/tern/issues/350
> for an example.

The format looks normal: the STATUS is expected to be a plist holding
the "history" of the connection.  It can contain various ":error FOO"
and ":redirect BAR" entries.

I think the problem comes from a doc error, where url-http points to
url-retrieve for the doc of CBARGS, whereas it uses the format of
url-retrieve-internal instead.


        Stefan

bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'

[Dmitry Gutov]

On 09/10/2014 06:24 AM, Stefan Monnier wrote:

> The problem is that calling `delete-process' may run the sentinel (and
> since this is code run from the sentinel, it may end up calling the
> sentinel recursively).

I can observe a definite change in behavior, which I cannot reason about 
(when connection fails, the external process dies in Emacs 24.3, but 
still lives for some time in 24.4). Someone else should judge whether 
this is a bug or an intended change.

> So if you don't want the sentinel to be called recursively, you'll want
> to (set-process-sentinel url-http-process nil) before calling
> delete-process

Yes, that's more or less what we did in Tern: 
https://github.com/marijnh/tern/commit/21245d5b901e6dc9cfb7c8ea55220a11104a5efc

>> If the callback expects STATUS to contain some specific data structure,
>> that can cause breakage, see https://github.com/marijnh/tern/issues/350
>> for an example.
>
> The format looks normal: the STATUS is expected to be a plist holding
> the "history" of the connection.  It can contain various ":error FOO"
> and ":redirect BAR" entries.

Indeed. Looks like my misunderstanding stemmed from Tern not handling 
this value exactly right: it expected STATUS to have a certain length in 
case of an error, and when called recursively, the callback received 
STATUS of different length, prepended with new history.

I guess there's no bug there, then. Sorry.

> I think the problem comes from a doc error, where url-http points to
> url-retrieve for the doc of CBARGS, whereas it uses the format of
> url-retrieve-internal instead.

Guess so. And both `url-http' and `url-retrieve-internal' are pretty 
confusing when it comes to describing the arguments that will be passed 
to CALLBACK.

To me, "When retrieval is completed, execute the function CALLBACK, 
using the arguments listed in CBARGS." means that it will be called 
exactly with the value of CBARGS passed to `url-http', whereas instead 
the list gets prepended with stuff before it's passed to CALLBACK.

bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'

[Stefan Monnier]

>> The problem is that calling `delete-process' may run the sentinel (and
>> since this is code run from the sentinel, it may end up calling the
>> sentinel recursively).
> I can observe a definite change in behavior, which I cannot reason about

I think there is indeed a change of behavior in that the sentinel used
to be nil'd while running it.
[ This was changed because it prevented the sentinel from modifying itself.  ]

> Indeed. Looks like my misunderstanding stemmed from Tern not handling this
> value exactly right: it expected STATUS to have a certain length in case of
> an error, and when called recursively, the callback received STATUS of
> different length, prepended with new history.

Right, it should use (cons nil <cbdata>) instead of (list <cbdata>),
since the `car' is used to carry the "historical events" of the request.

> I guess there's no bug there, then. Sorry.

Maybe not code-wise, but docstring-wise I think there's at least "room
for improvement".

> Guess so. And both `url-http' and `url-retrieve-internal' are pretty
> confusing when it comes to describing the arguments that will be passed
> to CALLBACK.
> To me, "When retrieval is completed, execute the function CALLBACK, using
> the arguments listed in CBARGS." means that it will be called exactly with
> the value of CBARGS passed to `url-http', whereas instead the list gets
> prepended with stuff before it's passed to CALLBACK.

Can you improve those docstrings, to avoid such confusion in the future?


        Stefan

bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'

[Dmitry Gutov]

Version: 24.3.93.4

On 09/12/2014 09:22 PM, Stefan Monnier wrote:

> I think there is indeed a change of behavior in that the sentinel used
> to be nil'd while running it.
> [ This was changed because it prevented the sentinel from modifying itself.  ]

Okay, I guess.

>> To me, "When retrieval is completed, execute the function CALLBACK, using
>> the arguments listed in CBARGS." means that it will be called exactly with
>> the value of CBARGS passed to `url-http', whereas instead the list gets
>> prepended with stuff before it's passed to CALLBACK.
>
> Can you improve those docstrings, to avoid such confusion in the future?

Done, I think (r117511). Although maybe we should remove the middle 
paragraph in `url-http' docstring and replace it with a reference to 
`url-retrieve-internal': its second sentence has very much the same 
contents, only using different words.