(In reply to comment #0)
> I've got a file descriptor leak.
> 
> I'm running Evolution, and one of my incoming filters pipes the incoming
> message to a shell command.  I find that this reliably leaks two FIFO fds,
> resulting in the need to restart evolution every few days when I exceed 1024
> open files.
> 
> I think I've tracked down the leak to camel-filter-search.c:run_command, which
> was recently refactored to use g_spawn_async_with_pipes().
> 
> I'm running fc5 with evolution-data-server-1.6.2-1.fc5.2
> 

Thanks for reporting the bug and tracking down the cause. :-)  Will be attaching a fix shortly.

Created attachment 71770 [details] [review]
Fixes fd leaks pointed out by Matt

Great.  I'll test this patch.

When I'd been digging around trying to fix this bug, I had thought that the combination:
        camel_stream_flush (stream);
        camel_object_unref (stream);
should arrange for the stream to close its file descriptors, via the
finalize method.  Isn't this the case?  Is something else not working?

*** Bug 348888 has been marked as a duplicate of this bug. ***

(In reply to comment #3)
> Great.  I'll test this patch.
> 
> When I'd been digging around trying to fix this bug, I had thought that the
> combination:
>         camel_stream_flush (stream);
>         camel_object_unref (stream);
> should arrange for the stream to close its file descriptors, via the
> finalize method.  Isn't this the case?  Is something else not working?
> 
Technically, Yes, this should close its file descriptors via the finalize method. Can you run valgrind with --trace-fds=yes and attach the log?

Also, if somebody has 2.4.x, try reproducing this bug, pls. 

perhaps the stream is being leaked? (e.g. the unref doesn't finalise the stream object because someone else holds a ref still)

I am marking the patch as obsolete, as technically, camel_object_unref of a stream calls its finalize and thus associated fds gets closed.

I tested the patch, and I found that evolution soon took 100% CPU, so I reverted to the previous version.

I've just run the unpatched version under valgrind.
It didn't appear to leak fds when running with memcheck, so I turned off
memcheck (--tool=none) and got several instances of the following:

==10665== Open file descriptor 64:
==10665==    at 0x51528D2: pipe (in /lib/libc-2.4.so)
==10665==    by 0x502AA84: g_child_watch_source_new (gmain.c:3620)
==10665==    by 0x42129E5: ??? (camel-filter-search.c:595)
==10665==    by 0x43DC130: e_sexp_term_eval (e-sexp.c:710)
==10665==    by 0x43DC3D7: ??? (e-sexp.c:440)
==10665==    by 0x43DC178: e_sexp_term_eval (e-sexp.c:700)
==10665==    by 0x421384C: ??? (camel-filter-search.c:327)
==10665==    by 0x43DC178: e_sexp_term_eval (e-sexp.c:700)
==10665==    by 0x43DCEC6: ??? (e-sexp.c:255)
==10665==    by 0x43DC178: e_sexp_term_eval (e-sexp.c:700)
==10665==    by 0x43DC1FF: e_sexp_eval (e-sexp.c:1304)
==10665==    by 0x4212671: camel_filter_search_match (camel-filter-search.c:705)

Hope this helps.

Created attachment 71859 [details] [review]
Fix for first-set-of-leaks

Fixes first set of leaks.  g_main_loop_new () ref's the context parameter and thus, context was never getting freed - memory leak + fd leak.

It seems the other leak is in glib.  Look at the following code snippet...

static void
g_child_watch_source_init_multi_threaded (void)
{
  GError *error = NULL;
  struct sigaction action;

  g_assert (g_thread_supported());

  if (pipe (child_watch_wake_up_pipe) < 0)
    g_error ("Cannot create wake up pipe: %s\n", g_strerror (errno));
  fcntl (child_watch_wake_up_pipe[1], F_SETFL, O_NONBLOCK | fcntl (child_watch_wake_up_pipe[1], F_GETFL));

....
....
....
}

The above code gets called for every g_child_watch_source_new ().  The pipe *child_watch_wake_up_pipe* is a static array that holds the fds-of-pipe and never gets closed.  (or atleast, I haven't seen it getting closed in glib/gmain.c).

But your snippet leaves out the line

  child_watch_init_state = CHILD_WATCH_INITIALIZED_THREADED;

and that *should* prevent the function from getting called a second time from g_child_watch_source_init(), as shown below:

static void
g_child_watch_source_init (void)
{
  if (g_thread_supported())
    {
      if (child_watch_init_state == CHILD_WATCH_UNINITIALIZED)
        g_child_watch_source_init_multi_threaded ();
      else if (child_watch_init_state == CHILD_WATCH_INITIALIZED_SINGLE)
        g_child_watch_source_init_promote_single_to_threaded ();
    }
  else
    {
      if (child_watch_init_state == CHILD_WATCH_UNINITIALIZED)
        g_child_watch_source_init_single ();
    }
}

So this part of GLib should only be leaking two file descriptors at most.  It wouldn't hurt to have another assertion or two in the logic though, if only for readability's sake:

static void
g_child_watch_source_init_multi_threaded (void)
{
  ...
  g_assert (g_thread_supported());
  g_assert (child_watch_init_state != CHILD_WATCH_INITIALIZED_THREADED);
  ...
}

Have you actually *seen* g_child_watch_source_init_multi_threaded() getting called multiple times, and if so what is the value of "child_watch_init_state" upon entry each time?

Thanks Veerapuram,

It looks as though your second patch has completely fixed my issue.

Matt

(In reply to comment #11)
> But your snippet leaves out the line
> 
>   child_watch_init_state = CHILD_WATCH_INITIALIZED_THREADED;
> 
Thanks for pointing out. :)

[snip...]
> So this part of GLib should only be leaking two file descriptors at most.  
Yes, two file descriptors are leaked for every application.

Closing the bug as Resolved fixed as per comment#12.  Rest of the leak is in glib.