bug#8293: mpc.el: "error in process filter"

bug#8293: mpc.el: "error in process filter"

[axel.junker]

I am using a semi-fresh build of emacs master on i686-pc-linux-gnu. This is a bug report concerning mpc.el. I have not modifier mpc.el or its variables so far.

When selecting an album in the "Albums | Playlists"-view (upper right corner), mpc.el issues a find command regarding Albums and Playlists
E.g. for an Album called 2001:

Send "(find Album 2001)"
Receive "file: dr. dre - 2001/Dr. Dre - Lolo.mp3
	Time: 42
	Artist: Dr. Dre
	Title: Lolo
[etc.]
	file: dr. dre - 2001/Dr. Dre - Bang Bang.mp3
	Time: 222
	Artist: Dr. Dre
	Title: Bang Bang
	Album: 2001
	Track: 21
	Genre: Hip-Hop
	OK
	"
Send "(listplaylistinfo 2001)"
Receive "ACK [50@0] {listplaylistinfo} No such playlist
	"

mpc--proc-filter regards "ACK [50@0] {listplaylistinfo} No such playlist" to be an error and signals an mpc-proc-error.
In mpc-cmd-find (mpc.el:599) the mpc-proc-cmd that issues that listplaylistinfo-request explicitly ignores any error by wrapping the invocation in a condition-case with a nil error handler.
Unfortunately this does not work. As the error is signaled by a process filter, emacs itself issues an error in process.c . That means, the signal never escapes the process filter. This leads to an ugly error output and the album selection does not work at all.

I think this is a bug.
/-Axel

In GNU Emacs 24.0.50.1 (i686-pc-linux-gnu, GTK+ Version 2.22.0)
 of 2011-03-03 on ghost
Windowing system distributor `The X.Org Foundation', version 11.0.10900000
Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Recent messages:
error in process filter: mpc--proc-filter: MPD error
error in process filter: MPD error

bug#8293: mpc.el: "error in process filter"

[Stefan Monnier]

> I am using a semi-fresh build of Emacs master on
> i686-pc-linux-gnu. This is a bug report concerning mpc.el. I have not
> modifier mpc.el or its variables so far.

> When selecting an album in the "Albums | Playlists"-view (upper right
> corner), mpc.el issues a find command regarding Albums and Playlists

> E.g. for an Album called 2001:

> Send "(find Album 2001)"
> Receive "file: dr. dre - 2001/Dr. Dre - Lolo.mp3
> 	Time: 42
> 	Artist: Dr. Dre
> 	Title: Lolo
> [etc.]
> 	file: dr. dre - 2001/Dr. Dre - Bang Bang.mp3
> 	Time: 222
> 	Artist: Dr. Dre
> 	Title: Bang Bang
> 	Album: 2001
> 	Track: 21
> 	Genre: Hip-Hop
> 	OK
> 	"
> Send "(listplaylistinfo 2001)"
> Receive "ACK [50@0] {listplaylistinfo} No such playlist
> 	"

> mpc--proc-filter regards "ACK [50@0] {listplaylistinfo} No such
> playlist" to be an error and signals an mpc-proc-error.

> In mpc-cmd-find (mpc.el:599) the mpc-proc-cmd that issues that
> listplaylistinfo-request explicitly ignores any error by wrapping the
> invocation in a condition-case with a nil error handler.
> Unfortunately this does not work. As the error is signaled by
> a process filter, emacs itself issues an error in process.c . That
> means, the signal never escapes the process filter. This leads to an
> ugly error output and the album selection does not work at all.
[...]
> Recent messages:
> error in process filter: mpc--proc-filter: MPD error
> error in process filter: MPD error

Hmm... it turns out that errors in process filters are handled
differently depending on debug-on-error (i.e. they're not caught
specially if debug-on-error is non-nil) so the above problem did not
show up in my use since I always set debug-on-error.

I think this indicates a problem in error handling of filters
(read_process_output should not catch errors if it's called by
accept-process-output), but since I'm not going to change this behavior
now, I installed the change below instead, which turns the filter error
into a process property which is turned into an error by the surrounding
code (the one that calls accept-process-output).

It seems to fix your immediate problem, tho it may introduce other
problems (in code that fails to check this new property).

Can you confirm your original problem is now fixed (album selection
still worked somehow in my tests, despite the error you pointed out)?


        Stefan


=== modified file 'lisp/mpc.el'
--- lisp/mpc.el	2011-06-02 11:24:12 +0000
+++ lisp/mpc.el	2011-08-24 01:42:56 +0000
@@ -246,11 +246,12 @@
             (process-put proc 'ready t)
             (unless (eq (match-end 0) (point-max))
               (error "Unexpected trailing text"))
-            (let ((error (match-string 1)))
+            (let ((error-text (match-string 1)))
               (delete-region (point) (point-max))
               (let ((callback (process-get proc 'callback)))
                 (process-put proc 'callback nil)
-                (if error (signal 'mpc-proc-error error))
+                (if error-text
+                    (process-put proc 'mpc-proc-error error-text))
                 (funcall callback)))))))))
 
 (defun mpc--proc-connect (host)
@@ -314,19 +315,23 @@
            mpc-proc)
       (setq mpc-proc (mpc--proc-connect mpc-host))))
 
+(defun mpc-proc-check (proc)
+  (let ((error-text (process-get proc 'mpc-proc-error)))
+    (when error-text
+      (process-put proc 'mpc-proc-error nil)
+      (signal 'mpc-proc-error error-text))))
+
 (defun mpc-proc-sync (&optional proc)
   "Wait for MPC process until it is idle again.
 Return the buffer in which the process is/was running."
   (unless proc (setq proc (mpc-proc)))
   (unwind-protect
-      (condition-case err
           (progn
             (while (and (not (process-get proc 'ready))
                         (accept-process-output proc)))
+        (mpc-proc-check proc)
             (if (process-get proc 'ready) (process-buffer proc)
-              ;; (delete-process proc)
               (error "No response from MPD")))
-        (error (message "MPC: %s" err) (signal (car err) (cdr err))))
     (unless (process-get proc 'ready)
       ;; (debug)
       (message "Killing hung process")

=== modified file 'src/process.c'
--- src/process.c	2011-08-18 15:33:22 +0000
+++ src/process.c	2011-08-24 01:50:01 +0000
@@ -5186,6 +5186,9 @@
 	  p->decoding_carryover = coding->carryover_bytes;
 	}
       if (SBYTES (text) > 0)
+	/* FIXME: It's wrong to wrap or not based on debug-on-error, and
+	   sometimes it's simply wrong to wrap (e.g. when called from
+	   accept-process-output).  */
 	internal_condition_case_1 (read_process_output_call,
 				   Fcons (outstream,
 					  Fcons (proc, Fcons (text, Qnil))),