I don't see any sync versions of g_app_info_launch() and g_app_info_launch_uris() that are used for spawning associated apps. Probably the only way would be to use GAppLaunchContext or GdkAppLaunchContext but that would require initializing Gtk. Not sure if it would help at all. There's no waitfor() and similar functions.

(In reply to comment #0)
> BTW: Bug 556305 indicates that gnome-open is deprecated in favour of gvfs-open.
>  Is that true, or should I file another bug report for gnome-open, too?
Yes, that's correct, gnome-open is dead already.

> Probably the only way would be to use GAppLaunchContext or GdkAppLaunchContext
> but that would require initializing Gtk.

People using gvfs-open are most likely GNOME (or XFCE) users, anyway, so would that be a big drawback?

kde's kde-open has a --tempfile option, which handles some(all?) the use-cases here, would implementing something like that help?

(In reply to comment #3)
> kde's kde-open has a --tempfile option, which handles some(all?) the use-cases
> here, would implementing something like that help?

I don't know what that option does exactly.  My main aim is to be able to put a call pattern into my ~/.mailcap that works for anything, so there mustn't be a need to chose different patterns for normal and temp files.

(In reply to comment #3)
> kde's kde-open has a --tempfile option, which handles some(all?) the use-cases
> here, would implementing something like that help?

I've now checked on a computer with kde what that option means:

KDE-tempfile options:
  --tempfile                The files/URLs opened by the application
                            will be deleted after use

That's not really what I'd need, and somehow it doesn't do what it says anyway.

  $ kde-open --tempfile foo.pdf
  # it does not return before the app (okular in this case) is opened
  $ ls foo.pdf
  foo.pdf

So the file is not deleted although the help text says so.

But in my use-case, the situation is that my mail app saves an attachment to /tmp/, then fires up gvfs-open <file>, and deletes that file after gvfs-open returns.  So I need a way to make gvfs-open not return before the spawned app returns.  Or as a workaround for wrapping gvfs-open in a shell script, I somehow need to get the PID of the launched app on that I can wait.

@Tomas: Checking the gvfs-open code and http://developer.gnome.org/gio/2.29/GAppInfo.html#g-app-info-launch it says that this functions sets the environment variable GIO_LAUNCHED_DESKTOP_FILE_PID.  But doing

  $ gvfs-open foo.pdf && echo $GIO_LAUNCHED_DESKTOP_FILE_PID

prints just the empty string.  That seems to be because NULL is provided as launch context...

So I've tried initializing one

  GAppLaunchContext *context = g_app_launch_context_new ();

and passing that in the 2 launch calls.  However, the command

  $ gvfs-open foo.pdf && echo $GIO_LAUNCHED_DESKTOP_FILE_PID

still only prints an empty line. :-(

I think you misunderstand how "kde-open --tempfile" works (the help text is wrong/misleading, imo).  Example:

cp foo.txt temp.txt; kde-open temp.txt ; rm -f temp.txt

will sometimes (depending on speed, race-conditions of course), give the "temp.txt not found" error, but

cp foo.txt temp.txt; kde-open --tempfile temp.txt ; rm -f temp.txt

will not (ie, it opens temporary *copy* of the file in question).

(In reply to comment #6)
> I think you misunderstand how "kde-open --tempfile" works (the help text is
> wrong/misleading, imo).  Example:
> 
> cp foo.txt temp.txt; kde-open temp.txt ; rm -f temp.txt
> 
> will sometimes (depending on speed, race-conditions of course), give the
> "temp.txt not found" error, but
> 
> cp foo.txt temp.txt; kde-open --tempfile temp.txt ; rm -f temp.txt
> 
> will not (ie, it opens temporary *copy* of the file in question).

Ah, ok.  That makes sense.  Not sure if that's a good thing, though.  I mean, my mail client already creates a copy which would be copied again.  That doesn't sound to scale very well if the file in question is big.  Well, that's not really an issue for the email attachment issue.  I hope nobody is ever going to send me a DVD image.

confirmed still an issue on fedora 16 using gvfs-1.10.1-2.fc16.x86_64

> So I need a way to make gvfs-open not return before the spawned app
> returns.  Or as a workaround for wrapping gvfs-open in a shell script, I
> somehow need to get the PID of the launched app on that I can wait.

Ok, for all suffering this problem, here's a shell script that I now use as universal open command in place of gvfs-open.  It queries gvfs to get the right application and then fires that up and waits till it finished.

There's one restriction: it assumes that a foo.desktop's file Exec command is foo.  In my case, that seems to be true for all apps I use, so I didn't bother sophisticating the script any further.

It's a ZSH script, but I think you can replace the shebang with bash as well, and it'll still work.

-----------------------------------------------------------------------
#!/bin/zsh

if [[ ${#} != 1 ]]; then
    echo "Usage: open <file>"
    exit 1
fi

mime_type=`gvfs-info 12-02-02-cpp-unicode.pdf | \
  grep "standard::content-type" | \
  cut -d' ' -f4`

# echo "The mime type of ${1} is ${mime_type}."

desktop_file=`gvfs-mime --query ${mime_type} | \
  grep "Default application for" | \
  cut -d: -f2 | tr -d ' '`

# echo "The default desktop file for ${mime_type} is ${desktop_file}."

command=`echo ${desktop_file} | sed -e s/\.desktop//`

${command} ${1}
-----------------------------------------------------------------------

Ups, of course, you need to replace the 12-02-02-cpp-unicode.pdf with ${1} in the script above.

Created attachment 216352 [details] [review]
gvfs-open: always use `g_app_info_launch_uris ()`

Created attachment 216353 [details] [review]
gvfs-open: add a `-w` flag to wait for the application to terminate

Please, find attached two patches to implement a `-w` flag to let gvfs-open waits for the application to terminate. It uses a deprecated function but I hope that the bug 678054 asking for a synchronous version of g_app_info_launch() will get a positive answer in the future.

Created attachment 216376 [details] [review]
gvfs-open: add a `-w` flag to wait for the application to terminate

We don't really need to record the PID of the launched application. We can use `waitpid(-1)` instead. We still need `g_desktop_app_info_launch_uris_as_manager()` because we need the `G_SPAWN_DO_NOT_REAP_CHILD` flag.

Review of attachment 216376 [details] [review]:

Note this patch will still break for applications that are single instance (via GtkApplication, libunique, or whatever).  The executed binary will send a message to the running instance and then exit.

There is a related problem also caused by gvfs-open exiting too early: if gvfs-open is run under a new pty, then it is the session group leader, and when it exits, its children are killed. There's a race condition: sometimes they can have had time to detach themselves (I've seen this happen when running gvfs-open under ltrace); other times, gvfs-open completes successfully, but the child is killed.

The fix is to have gvfs-open call setsid() to put it in its own process group.

I came across the problem when running gvfs-open from xdg-open (which is a shell script), and I was able to work around it by calling gvfs-open as "setsid gvfs-open". Of course, that doesn't help when gvfs-open is invoked directly.

I'd be happy to make a patch for gvfs-open.c if it is acceptable.

(I didn't file a separate bug because I thought it would quite possibly simply be marked as a dup; I could easily imagine making that mistake myself if I were a maintainer.)

(In reply to comment #15)
> I'd be happy to make a patch for gvfs-open.c if it is acceptable.

Please do so.

I attach the patch I proposed in comment #15. I stole the code from util-linux's setsid.c, as documented. You can see the original here: https://github.com/karelzak/util-linux/blob/master/sys-utils/setsid.c

Created attachment 293008 [details] [review]
Patch to fix accidental infanticide

Review of attachment 293008 [details] [review]:

Thanks for your patch and sorry for the delay. Not sure what this actually fixes and how to test it, seems not working for me... Please give me reproducer. 

Also the patch needs some changes.

Please follow the guidelines how to write commit message:
https://wiki.gnome.org/Git/CommitMessages

::: programs/gvfs-open.c
@@ +63,3 @@
+  /* setsid so that we do not accidentally kill children when we exit.
+     If necessary, first fork a child (see setsid(2)).
+     See bug #652262.

Please write preferably url instead of bug number.

@@ +65,3 @@
+     See bug #652262.
+     The following code was adapted from util-linux's setsid.c. */
+  if (getpgrp() == getpid()) {

Please use coding style as it is used in the file (so the curly brackets have to be on newlines, space have to be between function name and parentheses...).

@@ +69,3 @@
+    switch (pid) {
+    case -1:
+      g_printerr(_("fork failed"));

Would be nice to have those error messages in same format as others (so program name, description and new line). Wouldn't be better to use break here?

@@ +86,3 @@
+  }
+  if (setsid() < 0) {
+    /* cannot happen */

This comment doesn't make sence - why we are checking this if this can't happend?

Review of attachment 293008 [details] [review]:

::: programs/gvfs-open.c
@@ +82,3 @@
+        return WEXITSTATUS(status);
+      g_printerr(_("child %d did not exit normally"), pid);
+      return status;

Is undefined value returned here?

Thanks for the review. Before addressing it, it seems sensible to deal with your first point: "Not sure what this actually fixes and how to test it, seems not working for me... Please give me reproducer."

First, what is unclear about the original report, and the RedHat bug it links to?

Secondly, did you mean "seems working for me"? Or did you mean that the patch doesn't fix it for you?

Thirdly, are you saying that the recipe given in comment #1 does not reproduce the bug for you?

Once that is cleared up, I can rewrite the patch.

(In reply to comment #21)
> Thanks for the review. Before addressing it, it seems sensible to deal with
> your first point: "Not sure what this actually fixes and how to test it, seems
> not working for me... Please give me reproducer."
> 
> First, what is unclear about the original report, and the RedHat bug it links
> to?

Thanks for response. Sorry I wrongly interpreted your first sentence in Comment #15. I thought you are trying to fix just some subproblem. Original report seems clear to me...

> Secondly, did you mean "seems working for me"? Or did you mean that the patch
> doesn't fix it for you?

However it seems the patch doesn't fix it for me.

> Thirdly, are you saying that the recipe given in comment #1 does not reproduce
> the bug for you?

I can reproduce the problem using: 
echo foo > /tmp/test.txt; gvfs-open /tmp/test.txt; rm /tmp/test.txt

The file is opened empty, unfortunately with the patch also...

Maybe I'm facing also another bug, because sometimes the file isn't opened at all...

I'm sorry, I had forgotten the context since I originally posted the patch. Indeed, I am not trying here to fix the original bug. I tried to explain the problem I am fixing in comment #15. It is hard to give a recipe to reproduce it, because it is a race condition: as I said there, when gvfs-open is run under a new pty, sometimes it dies before its child process (or processes) have detached; since it is the process group leader, they are automatically killed.

I found this bug myself when using xdg-open from Emacs via M-x async-shell-command.

I hope this explains what I'm trying to achieve with the patch.

I attach a new version of the patch that tries to address your points.

In reply to your question, can an undefined value be returned, the answer is no: status is set by wait.

Created attachment 295050 [details] [review]
Patch to fix accidental infanticide

Updated following review.

I'm sorry also, I didn't read the comment 15 carefully. I think the fix is correct, but I'd like to find a way how to test it before committing...

Review of attachment 295050 [details] [review]:

It looks good otherwise, thanks.

The error message should be something like:
"gvfs-open: use setsid to avoid killing children

gvfs-open is the session group leader if it is run under a new pty,
therefore its children can be killed, when gvfs-open exits. The fix
is to call setuid() to put it in its own process group.

https://bugzilla.gnome.org/review?bug=652262"

::: programs/gvfs-open.c
@@ +71,3 @@
+        {
+        case -1:
+          g_printerr (_("%s: fork failed\n"), g_get_prgname ());

Would be good to add comments for translators before each translation to know what is the %s, something like:
/* Translators: the %s is the program name */

@@ +74,3 @@
+          break;
+
+       case 0:

wrong indentation (missing one space)

@@ +78,3 @@
+          break;
+
+       default:

dtto

Comment on attachment 216352 [details] [review]
gvfs-open: always use `g_app_info_launch_uris ()`

Marking the patch as obsolete, because the code was changed and we are currently using only g_app_info_launch_uris...

Created attachment 295099 [details] [review]
Patch to fix accidental infanticide

Updated as per review; thanks again!

I understand your wish to test the fix before committing. Do you use Emacs, or could you use it to perform the test I mentioned in comment #23? That fails reliably for me without the fix when opening a PDF file (which happens on my system with Evince).

If you're happy to try that test, and need more details of exactly what to do, do ask.

Review of attachment 295099 [details] [review]:

Ok, I've made first experience with emacs, hmm :-D So, I can reproduce it and the patch fixes it... You didn't fix the commit message as I wanted, but I suppose you don't have commit rights, so I will change it before commit.

Apologies, I had forgotten about fixing the commit message. I have updated the patch, changing only the message, as per the page you pointed me to above.

Created attachment 295305 [details] [review]
Avoid accidentally killing child processes

Review of attachment 295305 [details] [review]:

I don't think this is the correct solution because it is a workaround to
paper over a more fundamental design decision: should the started
process be treated like a daemon or as part of the current (shell)
session.  Also, if this change were done at all, it should really
be part of GAppInfo.

The processes are killed with SIGHUP because emacs closes the tty.

Either:
We should keep it as is, letting the processes handle SIGHUP how they
like, since they're still connected to the same session and controlling
tty.  Traditionally, one would use nohup before gvfs-open to make the child
process ignore the SIGHUP signal. (1)

Or:
Alternatively, g_app_info_launch_default_for_uri should run the child in
a clean state, as a child of init, in its own session and process group
(i.e. like a daemon).  Since gvfs-open would mostly
be used for running GUI apps, this is probably OK.  (But it kind of
contradicts the original bug which wants to wait for the app to exit.)


(1) Unfortunately, this doesn't currently work because the SIGHUP
disposition is set back to default for the child that is run.
I'm not sure why, I need to find out...

Note that this same sort of behavior can be reproduced quite simply:
In a gnome-terminal
$ eog somepicture.png &

and then close the terminal with the X.

Thanks for the analysis, Ross.

We have conflicting requirements: some users/uses want to start the child asynchronously (e.g. nautilus, or users "launching" files from the command line); some want to wait until it terminates (e.g. terminal programs like Mutt that want to launch helpers synchronously).

It might be argued that users should use "&" and programs running gvfs-open should fork, but it's too late: users and code already rely on the current behaviour.

Therefore, we should support both. For the asynchronous case, your suggestion that "g_app_info_launch_default_for_uri should run the child in a clean state" works nicely.

For the synchronous case, add a -w flag as suggested in Comment #12. As pointed out in Comment #14, a simple implementation won't work for single instance applications; support would have to be added to cover that case, such as already exists in Emacs as exploited by emacsclient.

The implementation approach is then threefold:

1. Run the child in a clean state.

2. Add a simple "-w" flag.

3. Add support for waiting on messages to single instance applications.

Some might consider 2 not worth doing without 3.

Finally, using nohup is not a good idea, for two reasons:

1. It creates an output file.

2. It makes child processes insensitive to NOHUP, rather than simply preventing NOHUP being propagated from gvfs-open to its children.

Further, consider the example in Comment #34: running "eog foo.png &" and then closing the terminal will ALWAYS close eog. However, running "gvfs-open foo.png" and then closing the terminal will close eog if it is done very quickly, but not if gvfs-open has already exited.

In other words, there's a definite race condition here, which should be solved. Both parts of the solution above fix it: when used asynchronously, gvfs-open will no longer cause the command it launches to be SIGHUP'd on exit (because it won't be in the same session); when used synchronously, gvfs-open will always still be there to be SIGHUP'd itself, and so the child will always get SIGHUP if gvfs-open's controlling tty is closed.

(When I said "We have conflicting requirements" above, I meant "There are conflicting requirements", not "You and I have conflicting requirements"!)

I had a look to see what might be involved in making these changes. Looking at the source to glib, in order to get the child to run in its own session, we could call g_desktop_app_info_launch_uris_as_manager with the setsid code I posted above, in a user_setup (GSpawnChildSetupFunc) function. (If application launching occurs via D-Bus, user_setup is ignored, but it will still be asynchronous, and there's no need for setsid.)

Since g_desktop_app_info_launch_uris_as_manager has no synchronous option, launching synchronously would require some other route or deeper patching.

The use of g_desktop_app_info_launch_uris_as_manager would prevent gvfs-open from working with any other implementations of GAppInfo. At present there don't seem to be any; I don't know how serious this limitation might prove; equally, I can't see a way to get the required functionality through the GAppInfo interface.

Indeed, the attached patch to implement -w has to wait for the launched process itself rather than using a synchronous launch.

As far as I can see, G[tk]Application has no functionality like Emacs's server-edit command (which notifies a client when all its buffers have been marked "done"), so that would take some work to define a new API and add it (and presumably this would be something to do via D-Bus).

New gio cmd tool has been added in GLib and thus GVfs cmd tools has been deprecated, see Bug 769378. Please test this issue against the new gio tool and file a new bug report under GLib product (GIO component) if it is still valid.

I filed bug #772397 after this, which became bug #1208 on GitLab: https://gitlab.gnome.org/GNOME/glib/issues/1208