bug#44824: [PATCH] org.el: Avoid xdg-open silent failure

[Kyle Meyer]

Maxim Nikulin writes:

> I hope, I have addressed other your comments in the updated patch.
>
> commit 5eca7764d94dd46b9f9a7792d1b786a3f03b20b6
> Author: Max Nikulin 
> Date:   Wed Feb 17 16:35:58 2021 +
>
> org.el: Avoid xdg-open silent failure

Thanks.  A note for future patches: your patch isn't in a format that's
ready to be consumed by git-am.  git-format-patch can help you here.

Applied to the Org repo (5db61eb0f), adding a TINYCHANGE cookie to the
commit message.  Please consider completing the copyright paperwork for
future patches
(see ).

If I understand correctly, this bug can be closed, but please reopen if
I'm mistaken.

Re: bug#44824: [PATCH] org.el: Avoid xdg-open silent failure

[Maxim Nikulin]

On 19/03/2021 10:50, Kyle Meyer wrote:

Maxim Nikulin writes:
A few comments in addition to Eli's advice to drop the
(eq system-type 'gnu/linux) condition...


Feel free to commit the change suggested in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44824#82
instead of this patch.


+(defun org--error-process-sentinel (proc event)
+  "Show a message if process failed (exited with non-zero code
+or killed by a signal.  Pass the function as :SENTINEL argument


Please rework the first sentence so that it fits on the first line,
though I'd be in favor dropping the function and using a lambda in the
make-process call.


My impression is that org-open-file function is already too long and 
complex. Another reason to use standalone function is that I am unsure 
if elisp compiler and interpreter are smart enough to reuse single 
instance of lambda. I was afraid that every opened file caused creation 
of new sentinel possibly with a closure containing chain of stack 
frames. On the other hand even in worst case memory footprint is 
negligible in comparison to any GUI viewer.



+  (unless (string-match "finished" event)


There's no need for substring matching, right?  So it could be

   (equal event "finished\n")


I was surprised by final "\n" that is not always suitable and I was in 
doubts concerning its stability. I would prefer something like


(starts-with event "finished")

Certainly match-data is not necessary, so even match-string-p is better.


   (when (and (memq (process-status proc) '(exit signal))
  (/= (process-exit-status proc) 0))


Thank you, I was too lazy to implement such kind of check myself. 
Certainly this variant is better.


I hope, I have addressed other your comments in the updated patch.

commit 5eca7764d94dd46b9f9a7792d1b786a3f03b20b6
Author: Max Nikulin 
Date:   Wed Feb 17 16:35:58 2021 +

org.el: Avoid xdg-open silent failure

* lisp/org.el (org-open-file): Use 'pipe :connection-type instead of
'pty to prevent killing of background process on handler exit.
(Bug#44824)

Problem happens only in some desktop environments where configured
through `org-file-apps' or mailcap handlers launches actual viewer
(as defined in .desktop files and obtained from mimeapps.list)
in background.  E.g. xdg-open invokes "gio open" or kde-open5 for Gnome
or KDE accordingly and these handlers launches e.g. eog or okular in
background.  As soon as main process exits, temporary terminal session
created by `start-process-shell-command' is terminated.  As a result
background processes receive SIGHUP.

Previously command were executed with no buffer, so the change
does not affect "needsterminal" and "copiousoutput" mailcap features,
they are not supported as earlier.

If handler main process fails then show a message with exit reason.
Output (including error messages) is ignored as before.
Gtk application tends to report significant amount of failed asserts
hardly informative for majority of users.

diff --git a/lisp/org.el b/lisp/org.el
index 4db2dbe04..c58708a5f 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -8768,7 +8768,21 @@ If the file does not exist, throw an error."
 
   (save-window-excursion
 	(message "Running %s...done" cmd)
-	(start-process-shell-command cmd nil cmd)
+;; Handlers such as "gio open" and kde-open5 start viewer in background
+;; and exit immediately.  Avoid `start-process' since it assumes
+;; :connection-type 'pty and kills children processes with SIGHUP
+;; when temporary terminal session is finished.
+(make-process
+ :name "org-open-file" :connection-type 'pipe :noquery t
+ :buffer nil ; use "*Messages*" for debugging
+ :sentinel (lambda (proc event)
+ (when (and (memq (process-status proc) '(exit signal))
+(/= (process-exit-status proc) 0))
+   (message
+"Command %s: %s."
+(mapconcat #'identity (process-command proc) " ")
+(substring event 0 -1
+ :command (list shell-file-name shell-command-switch cmd))
 	(and (boundp 'org-wait) (numberp org-wait) (sit-for org-wait
  ((or (stringp cmd)
 	  (eq cmd 'emacs))

bug#44824: [PATCH] org.el: Avoid xdg-open silent failure

[Kyle Meyer]

Maxim Nikulin writes:

> org.el: Avoid xdg-open silent failure
> 
> * lisp/org.el (org-open-file): Use 'pipe :connection-type instead of
> 'pty to prevent killing of background process on handler exit.
> 
> Problem happens only in some desktop environments where configured
> through `org-file-apps' or mailcap handlers launches actual viewer
> (as defined in .desktop files and obtained from mimeapps.list)
> in background.  E.g. xdg-open invokes "gio open" or kde-open5 for Gnome
> or KDE accordingly and these handlers launches e.g. eog or okular in
> background.  As soon as main process exits, temporary terminal session
> created by `start-process-shell-command' is terminated.  As a result
> background processes receive SIGHUP.
> 
> Previously command were executed with no buffer, so the change
> does not affect "needsterminal" and "copiousoutput" mailcap features,
> they are not supported as earlier.
> 
> If handler main process fails then show a message with exit reason.
> Output (including error messages) is ignored as before.
> Gtk application tends to report significant amount of failed asserts
> hardly informative for majority of users.

Thanks for the detailed commit message.

A few comments in addition to Eli's advice to drop the
(eq system-type 'gnu/linux) condition...

> diff --git a/lisp/org.el b/lisp/org.el
> index 7d8733448..a199a65c9 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -8645,6 +8645,15 @@ opened in Emacs."
> (when add-auto-mode
>   (mapcar (lambda (x) (cons (car x) 'emacs)) auto-mode-alist
>  
> +(defun org--error-process-sentinel (proc event)
> +  "Show a message if process failed (exited with non-zero code
> +or killed by a signal.  Pass the function as :SENTINEL argument

Please rework the first sentence so that it fits on the first line,
though I'd be in favor dropping the function and using a lambda in the
make-process call.

> +of `make-process'."
> +  (unless (string-match "finished" event)

There's no need for substring matching, right?  So it could be

  (equal event "finished\n")

Or perhaps

  (when (and (memq (process-status proc) '(exit signal))
 (/= (process-exit-status proc) 0))
...)

> +(message "Command %s: %s."
> + (mapconcat 'identity (process-command proc) " ")

s/'identity/#'identity/

> + (substring event 0 -1
> +
>  ;;;###autoload
>  (defun org-open-file (path  in-emacs line search)
>"Open the file at PATH.
> @@ -8766,7 +8775,17 @@ If the file does not exist, throw an error."
>  
>(save-window-excursion
>   (message "Running %s...done" cmd)
> - (start-process-shell-command cmd nil cmd)
> + (if (eq system-type 'gnu/linux)
> +   ;; Handlers as "gio open" and kde-open5 start viewer in background

s/as/such as/ ?

> +   ;; and exit immediately. Avoid start-process since it assumes

  ^ missing space

> +   ;; :connection-type 'pty and kills children processes with SIGHUP
> +   ;; when temporary terminal session is finished.
> +   (make-process
> + :name "org-open-file" :connection-type 'pipe :noquery 't

s/'t/t/

> + :buffer nil ; use "*Messages*" for debugging
> + :sentinel 'org--error-process-sentinel
> + :command (list shell-file-name shell-command-switch cmd))
> +   (start-process-shell-command cmd nil cmd))
>   (and (boundp 'org-wait) (numberp org-wait) (sit-for org-wait
>   ((or (stringp cmd)
> (eq cmd 'emacs))

Thanks.

Re: bug#44824: [PATCH] org.el: Avoid xdg-open silent failure

[Maxim Nikulin]

On 19/02/2021 21:54, Eli Zaretskii wrote:

From: Maxim Nikulin
Date: Fri, 19 Feb 2021 19:29:49 +0700


On Windows Emacs always uses pipes, because we don't have PTYs there.
And there's no xdg-open on MS-Windows anyway, so it's a moot point.


Should I consider your response as a suggestion to remove the `if'
related to `system-type'?


Yes, that 'if' isn't necessary.


Let's wait for comments comments from org developers. I do not mind to 
remove the `if' but they could ask for other changes.



If I remember correctly, on windows it is possible to communicate with a
process through stdin and stdout only if the application is compiled as
a *console* one.


That's true.  But in this case we don't really want to communicate
with the sub-process, do we?  We just want to invoke it and let it
run.  So the fact that there's no way of communicating with the
sub-process is not important here, as the pipes will not be used.  We
just need to specify pipes because that works around the problem with
xdg-open.


The reason to switch to pipe process here is to avoid side effects of 
*terminal session*. PTY is redundant here. Pipe process is the only 
alternative available in emacs. Currently there is no data exchange with 
subprocess.


Reading of stderr and stdout from the handler and its descendants could 
be useful to realize a cause of a problem when it happens. At least on 
linux it is enough to specify some buffer instead of `nil' or add a 
:filter function. The reason why I did not make such change is that Gtk 
applications tends to report significant number of failed asserts. I do 
not like to pollute the "*Messages*" buffer. I am in doubts if a 
dedicated buffer should be created for such purpose.


Reading of stdout could be useful in mailcap handlers e.g. to display 
text from PDF file when only text terminal is available (no X11 or 
wayland). Currently emacs ignores "needsterminal" and "copiousoutput" 
flags in mailcap. Example of an entry for PDF:


application/pdf; pdftotext -layout %s -; test=test -z "$DISPLAY"; 
copiousoutput


Likely only a small part of users could benefit from such feature.


"start file.pdf" executed in cmd.exe launches an application that does
not block command prompt. In this sense it similar to background
processes launched by kde-open5 or "gio open". However I am unaware if
there is something similar to process groups on windows that leads to
termination of all group members when leader process finishes.


Things are fairly similar on Windows.  But is this really relevant to
the issue at hand?  There's no xdg-open on Windows, so whatever
problems you had with xdg-open will never happen on Windows.  the
proposed patch fixes the problem only on systems where org.el invokes
the PDF viewer via xdg-open.  Right?


1. It is not a problem of namely xdg-open. It is (mostly reasonable but 
with some caveats however) specific of DE-specific handlers as "gio 
open" and kde-open5. They exit just after spawning of a process with 
actual handler. (Just like "start file.pdf" on windows.) I am afraid 
that I could break something on windows, so I would prefer that somebody 
will test the changes on non-linux systems.


2. It is not specific to PDF files, the problem could happen with any 
format if wrapper handler such as "gio open" is invoked instead of e.g. 
direct execution of eog.

bug#44824: [PATCH] org.el: Avoid xdg-open silent failure

[Eli Zaretskii]

> From: Maxim Nikulin 
> Date: Fri, 19 Feb 2021 19:29:49 +0700
> Cc: 44...@debbugs.gnu.org
> 
> > On Windows Emacs always uses pipes, because we don't have PTYs there.
> > And there's no xdg-open on MS-Windows anyway, so it's a moot point.
> 
> Should I consider your response as a suggestion to remove the `if' 
> related to `system-type'?

Yes, that 'if' isn't necessary.

> If I remember correctly, on windows it is possible to communicate with a 
> process through stdin and stdout only if the application is compiled as 
> a *console* one.

That's true.  But in this case we don't really want to communicate
with the sub-process, do we?  We just want to invoke it and let it
run.  So the fact that there's no way of communicating with the
sub-process is not important here, as the pipes will not be used.  We
just need to specify pipes because that works around the problem with
xdg-open.

> "start file.pdf" executed in cmd.exe launches an application that does 
> not block command prompt. In this sense it similar to background 
> processes launched by kde-open5 or "gio open". However I am unaware if 
> there is something similar to process groups on windows that leads to 
> termination of all group members when leader process finishes.

Things are fairly similar on Windows.  But is this really relevant to
the issue at hand?  There's no xdg-open on Windows, so whatever
problems you had with xdg-open will never happen on Windows.  the
proposed patch fixes the problem only on systems where org.el invokes
the PDF viewer via xdg-open.  Right?

Re: bug#44824: [PATCH] org.el: Avoid xdg-open silent failure

[Maxim Nikulin]

On 18/02/2021 21:48, Eli Zaretskii wrote:

From: Maxim Nikulin
Date: Thu, 18 Feb 2021 19:56:03 +0700

I could not estimate effect of such change on windows, so pipe process
is used only on linux. I am unsure concerning mac however.


On Windows Emacs always uses pipes, because we don't have PTYs there.
And there's no xdg-open on MS-Windows anyway, so it's a moot point.


Should I consider your response as a suggestion to remove the `if' 
related to `system-type'?


If I remember correctly, on windows it is possible to communicate with a 
process through stdin and stdout only if the application is compiled as 
a *console* one. That is why it is unclear for me, which component 
handles console or GUI distinction when process is specified as pipe in 
lisp code, and whether all arguments of `make-process', that I have 
used, are suitable on windows.


"start file.pdf" executed in cmd.exe launches an application that does 
not block command prompt. In this sense it similar to background 
processes launched by kde-open5 or "gio open". However I am unaware if 
there is something similar to process groups on windows that leads to 
termination of all group members when leader process finishes.

bug#44824: [PATCH] org.el: Avoid xdg-open silent failure

[Eli Zaretskii]

> From: Maxim Nikulin 
> Date: Thu, 18 Feb 2021 19:56:03 +0700
> Cc: 44...@debbugs.gnu.org
> 
> I could not estimate effect of such change on windows, so pipe process
> is used only on linux. I am unsure concerning mac however.

On Windows Emacs always uses pipes, because we don't have PTYs there.
And there's no xdg-open on MS-Windows anyway, so it's a moot point.