* bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'
@ 2014-07-27 3:12 Dmitry
2014-08-13 17:06 ` Glenn Morris
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Dmitry @ 2014-07-27 3:12 UTC (permalink / raw
To: 18116
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'
2014-07-27 3:12 bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process' Dmitry
@ 2014-08-13 17:06 ` Glenn Morris
2014-08-18 19:34 ` Óscar Fuentes
2014-09-10 2:24 ` Stefan Monnier
2 siblings, 0 replies; 7+ messages in thread
From: Glenn Morris @ 2014-08-13 17:06 UTC (permalink / raw
To: Dmitry; +Cc: 18116
Maybe you could bisect for the cause?
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'
2014-07-27 3:12 bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process' Dmitry
2014-08-13 17:06 ` Glenn Morris
@ 2014-08-18 19:34 ` Óscar Fuentes
2014-09-10 2:24 ` Stefan Monnier
2 siblings, 0 replies; 7+ messages in thread
From: Óscar Fuentes @ 2014-08-18 19:34 UTC (permalink / raw
To: 18116
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'
2014-07-27 3:12 bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process' Dmitry
2014-08-13 17:06 ` Glenn Morris
2014-08-18 19:34 ` Óscar Fuentes
@ 2014-09-10 2:24 ` Stefan Monnier
2014-09-11 1:52 ` Dmitry Gutov
2 siblings, 1 reply; 7+ messages in thread
From: Stefan Monnier @ 2014-09-10 2:24 UTC (permalink / raw
To: Dmitry; +Cc: 18116
> (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
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'
2014-09-10 2:24 ` Stefan Monnier
@ 2014-09-11 1:52 ` Dmitry Gutov
2014-09-12 17:22 ` Stefan Monnier
0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Gutov @ 2014-09-11 1:52 UTC (permalink / raw
To: Stefan Monnier; +Cc: 18116
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'
2014-09-11 1:52 ` Dmitry Gutov
@ 2014-09-12 17:22 ` Stefan Monnier
2014-09-19 2:00 ` Dmitry Gutov
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Monnier @ 2014-09-12 17:22 UTC (permalink / raw
To: Dmitry Gutov; +Cc: 18116
>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process'
2014-09-12 17:22 ` Stefan Monnier
@ 2014-09-19 2:00 ` Dmitry Gutov
0 siblings, 0 replies; 7+ messages in thread
From: Dmitry Gutov @ 2014-09-19 2:00 UTC (permalink / raw
To: Stefan Monnier; +Cc: 18116-done
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-09-19 2:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-27 3:12 bug#18116: 24.3.92; url-http calls CALLBACK recursively with malformed CBARGS if the former calls `delete-process' Dmitry
2014-08-13 17:06 ` Glenn Morris
2014-08-18 19:34 ` Óscar Fuentes
2014-09-10 2:24 ` Stefan Monnier
2014-09-11 1:52 ` Dmitry Gutov
2014-09-12 17:22 ` Stefan Monnier
2014-09-19 2:00 ` Dmitry Gutov
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.