Yeah, didn't have to look long to notice a serious bug in gspawn-
win32.c: g_spawn_async() in fact worked synchronously. There was 
confusion about the semantics of the G_SPAWN_DO_NOT_REAP_CHILD flag. 
Fix committed to HEAD and glib-2-0.

Will continue to look for bugs. Just performance enhancements should 
go only in HEAD, I assume.

Found another problem: The asynchronous g_spawn* functions are 
supposed to  the pid of the spawned process. In fact, they returned 
zero.

Fixing this was more complex than I thought at first. Firstly, the 
spawn*() functions in the C runtime return a process handle, not a 
process identifier. Oh well, the handle is more useful, actually. 
Secondly, the helper process has to pass the handle up to the caller. 
Well, the same pipe where error codes are passed can be used. 
Thirdly, the process handle returned from spawn*() is local to the 
process that called spawn*(), i.e. the helper process. To be valid in 
the original process, it has to be duplicated with DuplicateHandle().

Fix committed to HEAD and glib-2-0.

Tor - could you close this bug if there are no outstanding issues,
otherwise assign it to an appropriate milestone?

(2.2.0 if you think you'll fix more in the next couple of weeks;
2.2.1 if there are small changes to make, 2.4.0 if further 
work would be major changes)

Assigned to 2.2.0 and severity set to enhancement; there are no 
outstanding bugs I know, but there is room for some performance 
improvements (get rid of the helper program in easy cases).

Changing milestone to 2.4.0, probably won't have time to do the 
enhancements soon enough for 2.2.0.

Above there is some talking about modifiying The Gimp 1.2
to test g_spawn_aysnc. Did you do it ? Gimp 1.3 did need
some patch like below to work rasonable again. 
There are two major issues :
- Querying plug-ins did not work at all on win9x and almost
  hangs the system on NT
- To allow scripting (pygimp) some extendig is needed
And obviously it should be reasonable fast to work with about
140 plug-ins ...

gboolean
g_spawn_async (const gchar          *working_directory,
               gchar               **argv,
               gchar               **envp,
               GSpawnFlags           flags,
               GSpawnChildSetupFunc  child_setup,
               gpointer              user_data,
               gint                 *child_pid,
               GError              **error)
{
  char executable[MAX_PATH];
  gint pid;

  g_return_val_if_fail (argv != NULL, FALSE);

  if (child_setup)
    {
      (* child_setup) (user_data);
    }

  /* first try: simply execute */
  pid = _spawnve (_P_NOWAIT, argv[0], argv, envp);
  if (-1 == pid)
    {
      if (32 <= (int)FindExecutable (argv[0], working_directory,
executable))
        {
          /* e.g. needed to execute Gimp Python plug-ins, which have the
           * executable flag set on *nix. There is no such on win32. */
#define _MAX_CMD_LEN 1024
          /* stuff parameters into one cmndline */
          char cmdline[_MAX_CMD_LEN];
          int i, len = 0;

          cmdline[0] = 0;
          for (i = 0; argv[i] != NULL; i++)
            {
               len += strlen(argv[i]) + 1;
               if (len >= _MAX_CMD_LEN)
                 {
                    g_set_error (error, G_SPAWN_ERROR,
G_SPAWN_ERROR_FAILED,
                                 _("Command line length exceeds
internal limit."));
                   return FALSE;
                 }
               strcat (cmdline, argv[i]);
               strcat (cmdline, " ");
            }
          /* remove last blank */
	  if (len > 0)
            cmdline[len - 1] = 0;

          pid = _spawnlp (_P_NOWAIT, executable, "-c", cmdline, NULL);
          if (-1 == pid)
            {
              g_set_error (error, G_SPAWN_ERROR, G_SPAWN_ERROR_FAILED,
                           _("Found executable '%s' but failed to
execute."),
                           executable);
              return FALSE;
            }
#undef _MAX_CMD_LEN
        }
    }

  if ((G_SPAWN_DO_NOT_REAP_CHILD & flags) && (child_pid))
    *child_pid = pid;
  else
    CloseHandle ((HANDLE)pid);

  if (-1 != pid)
    return TRUE;

  return g_spawn_async_with_pipes (working_directory,
                                   argv, envp,
                                   flags,
                                   child_setup,
                                   user_data,
                                   child_pid,
                                   NULL, NULL, NULL,
                                   error);
}

Yes, I did try a modified GIMP 1.2.3 that used g_spawn_async(), and 
it did work (once I fixed some bugs as mentioned). But this was on 
Win2k... I don't have any Win9x system to test on.

As for the script support, yes, that definitely is something that 
should go in. I don't know if putting it in g_spawn_async() is 
correct, though, shouldn't it also be possible to run scripts 
synchronously?

The slowness of GIMP running (all) its plug-ins is an issue only the 
first time it is started, isn't it? After that it runs plug-ins only 
as needed, or when they have been changed or added. Maybe the GIMP 
installer(s) on Win32 should ship with a pre-configured pluginrc file 
corresponding to the plug-in executables included with the installer, 
then they wouldn't have to be queried at first-time startup.

Changing target milestone to 2.2.3...

Partly fixed in HEAD and glib-2-2:

2003-07-31  Tor Lillqvist  <tml@iki.fi>

 * glib/gspawn-win32.c: When possible, manage without the helper
 process. (Part of the enhancements outlined in #98737.) Speeds up
 GIMP 1.3's first-time-run plug-in query phase a lot.

 Plug a file descriptor (and thus Win32 handle) leak: close the
 read end of the child error report pipe after use.

We could also avoid the helper process even if a new working 
directory for the child process is specified: If this is not a multi-
threaded process, we could chdir() there, start the child, and chdir
() back. 

Also in single-threaded processes, the G_SPAWN_STDOUT_TO_DEV_NULL and 
G_SPAWN_STDERR_TO_DEV_NULL flags could be implemented without a 
helper process by dup()ing fds 1 and 2, closing them, starting the 
child, and then re-duping back to 1 and 2 (and close the dups). Hmmm.

Script support still needs to be added, will look at that next.

To make other (e.g. Python) scripts available to The Gimp some change
similar to the proposed above is still needed - so IMO it is not just
an enhancement.

Created attachment 24891 [details] [review]
updated patch against current CVS

Can something like this get in glib (2.4.x preferably) soon? It'd be nice to
have python/perl/etc. stuff working on windows. Hans's patch seems fine, other
than it should go in do_spawn rather than g_spawn_async (not needed for gimp,
but there should be some consistency).

The patch will have to be worked on a bit in order not to cause regressions. 

Especially the direct attempt at spawnve() first looks dangerous. Consider the 
protect_argv stuff, for instance. Also the use of "-c" seems odd, isn't that 
typically used by Unixish shells to give them a single *command* to interpret, 
not the name of a script file? (At least on my machine, running 'assoc .py' 
gives 'Python.File', and running 'ftype Python.File' 
gives 'Python.File=F:\Progs\Python22\python.exe "%1" %*'.)

I guess the two places in gspawn-win32.c and gspawn-win32-helper.c where spawnvp
() or spawnv() are called should be replaced with calls to some function that 
first tries the spawnvp/spawnv(), then if it fails, checks if the name of the 
executable ends with one of the suffixes in %PATHEXT%, and in that case calls 
ShellExecute()?

I'm using the patch since about one and a half year - but maybe that's the
reason I didn't notice any regression. Also the only application on my computer
using g_spawn* is The Gimp. I agree the '-c' looks odd but I didn't manage to
get pygimp plug-ins to work without it. Here's what the "python -h" says :

usage: D:\DEVEL\PYTHON22\PYTHON.EXE [option] ... [-c cmd | file | -] [arg] ...
Options and arguments (and corresponding environment variables):
-c cmd : program passed in as string (terminates option list)

[removed myself from cc, to avoid to get further comments twice]

Have you checked your patch with argv elements with spaces or double quotes in 
them? How is calling _spawnve() directly from g_spawn_async() supposed to 
handle changing directory for the subprocess? Or the various G_SPAWN_* flags 
that can be set for g_spawn_async()? 

Hmm, no response in 2 years. Should we close this ?

Yeah. It's hard to say what this bug report is about any longer.