challenges creating processes from threaded environment?

[Jeff Trawick <tr...@attglobal.net>]

consider code that does something like the following:

apr_procattr_io_set()
/* we now have 6 pipe handles with no cleanup-for-exec on any */
apr_proc_create()
/* parent now has 3 of the pipe handles with no cleanup-for-exec on any */
apr_pool_cleanup_register(parent handle to child's stdin,
                           child cleanup to close file)
/* now we know that other processes created between now and when we're 
through writing to child's stdin won't inherit the handle too and 
prevent the child from seeing EOF when this thread closes it */

But this entire sequence needs to be in a critical section because any 
other code in the process doing an apr_proc_create() at about the same 
time can inherit this child's handles and cause problems.

I encountered this problem in Apache's mod_ext_filter, which spawns a 
child process to filter a response from the server.  Banging on the 
server and having it spawn sed to filter the response left a number of 
sed processes hanging around, all waiting for EOF on stdin.  But each 
sed's stdin pipe was led open by another sed process, so it mattered not 
that Apache had closed its side of the pipe since the pipe was still open.

For mod_ext_filter I can hold a thread mutex around the critical section 
  above, and that protects it from other instances of mod_ext_filter 
doing fork() and inheriting the wrong handle, but it does nothing for 
other threads calling apr_proc_create().

It would seem that all of the pipe creation and child cleanup 
registration needs to be done inside apr_proc_create() so that it can 
hold a mutex and all callers of apr_proc_create() will be happy.  I 
didn't look at it to know if that would require API changes.  I wouldn't 
be shocked.

Thoughts?

Re: apr_dso_load() suffixes

[Wan-Teh Chang <wt...@netscape.com>]

Jeff Trawick wrote:

 > can't see such code...  maybe Win32 itself adds .dll under some
 > circumstances

This is documented in the Win32 LoadLibrary() function's
documentation.  Windows adds .dll if the specified file
name has no extension.  If you want to load a DLL whose
name really has no extension, you need to explicitly
append a dot (.) to the DLL's name.

See the Remarks section in
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/loadlibrary.asp.

Wan-Teh

Re: apr_dso_load() suffixes

[Jeff Trawick <tr...@attglobal.net>]

Marc M. Adkins wrote:
> It seems like on Unix apr_dso_load() requires a suffix for the pathname

I don't see any code in apr_dso_load() that requires that.  Perhaps your 
OS has a requirement?

> but
> on Windows it will provide one (.dll).

can't see such code...  maybe Win32 itself adds .dll under some 
circumstances

>  WRT writing portable code...it looks
> like Apache itself just uses .so on all platforms

the Apache build makes some effort to name them mod_foo.so

 >...or is there some other
> mechanism?  For example, a place where the appropriate platform-specific
> suffix might be acquired?

apr_dso_load() suffixes

["Marc M. Adkins" <mm...@Doorways.org>]

It seems like on Unix apr_dso_load() requires a suffix for the pathname but
on Windows it will provide one (.dll).  WRT writing portable code...it looks
like Apache itself just uses .so on all platforms...or is there some other
mechanism?  For example, a place where the appropriate platform-specific
suffix might be acquired?

mma

Re: challenges creating processes from threaded environment?

[Jeff Trawick <tr...@attglobal.net>]

another general problem:

between the time a descriptor is allocated by the kernel (by open(), 
pipe(), socket(), accept(), etc.) and the time that a child cleanup can 
be registered for it, apr_proc_create() could have fork()-ed and allowed 
a descriptor to be inappropriately inherited

in order to fix this problem in general:

Establish a critical section covering the path between descriptor 
allocation and registration of the cleanup; any number of threads 
allocating descriptors can enter that critical section at any given 
time, but a thread performing a fork() must have exclusive access 
(read-only access vs. read-write access).

APIs that allocate descriptors may allow the caller to specify whether 
or not the file should be inherited, and APIs without such a parameter 
default to not-inherited and the user can call apr_foo_inherit_set(), as 
with apr_file_open() currently.

issues:

open() and accept() are potentially blocking...  we can't hold the mutex 
for this critical section across an open() or accept() since that could 
deadlock the APR application.  Thus, the descriptor would be allocated 
by the kernel and inherited by new processes created by other threads in 
the window prior to registering a child cleanup.

hack for open(): disable blocking on the open of a named pipe by passing 
O_NONBLOCK (not without side-effects)

hack for accept() on a blocking socket: make it non-blocking 
temporarily, wait in select() or poll() until appropriate time, get 
mutex, call accept(), release mutex

--/--

back to the original complaint about pipes created by 
apr_procattr_io_set() and the related apr_proc_create()...

the client side of those pipes actually must be inherited by the one new 
process intended to use them, so whatever was done to avoid arbitrary 
processes inheriting them must be undone for the intended process in 
apr_proc_create() while holding the critical section

Re: challenges creating processes from threaded environment?

[Jeff Trawick <tr...@attglobal.net>]

William A. Rowe, Jr. wrote:
> At 08:02 PM 5/23/2003, Jeff Trawick wrote:

> OWWW!  That's a case I hadn't considered.  However, the right answer
> is to close the children's side of the pipes, always.

apr_proc_create() does that unconditionally in the parent path after fork()

it should be changed to register a cleanup-for-exec on the parent's side 
of the pipes...  I suspect that this particular hole can be filled 
immediately before mutexing and related API compatibility concerns are 
worked out, unless somebody can come up with what sort of app this would 
break

>  If they are left open,
> it's much harder to detect that the child has died/you've reached EOF.
> 
> 
>>For mod_ext_filter I can hold a thread mutex around the critical section  above, 
 >and that protects it from other instances of mod_ext_filter doing fork()
 >and inheriting the wrong handle, but it does nothing for other threads
 >calling apr_proc_create().
> 
> 
> The right answer, in part, is to close those child handles.

here I'm confused because it looks like apr_proc_create() already does that

>  The second side
> is to protect that window where we declare the handles as keep-open on
> exec to the point where we've already fork()ed.

agreed

>>It would seem that all of the pipe creation and child cleanup 
 >registration needs to be done inside apr_proc_create() so that
 >it can hold a mutex and all callers of apr_proc_create() will
 >be happy.  I didn't look at it to know if that would require
 >API changes.  I wouldn't be shocked.
> 
> 
> For the small window between declaring the handles keep-open,
> fork()ing the child and then closing them in the parent, we absolutely
> need a mutex to keep things clean.

yes; basically, get a mutex at start of apr_proc_create, then create 
pipes as noted in apr_procattr_io_set(), then fork() then close child 
side of handles and register cleanup-on-exec for other handles,
then release mutex

so apr_procattr_io_set() is changed to note what to do later in 
apr_proc_create() instead of actually creating the pipes

all the while, keep track of which if any function is permanently lost 
this way and also which changes require potential adjustment in the app :)

Re: challenges creating processes from threaded environment?

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

At 08:02 PM 5/23/2003, Jeff Trawick wrote:
>consider code that does something like the following:
>
>apr_procattr_io_set()
>/* we now have 6 pipe handles with no cleanup-for-exec on any */
>apr_proc_create()
>/* parent now has 3 of the pipe handles with no cleanup-for-exec on any */
>apr_pool_cleanup_register(parent handle to child's stdin,
>                          child cleanup to close file)
>/* now we know that other processes created between now and when we're through writing to child's stdin won't inherit the handle too and prevent the child from seeing EOF when this thread closes it */
>
>But this entire sequence needs to be in a critical section because any other code in the process doing an apr_proc_create() at about the same time can inherit this child's handles and cause problems.

100% correct.  I was looking at this a couple of weeks ago, but hadn't
formulated any obvious answers to it (without seriously throttling all
multithreaded apps.)

>I encountered this problem in Apache's mod_ext_filter, which spawns a child process to filter a response from the server.  Banging on the server and having it spawn sed to filter the response left a number of sed processes hanging around, all waiting for EOF on stdin.  But each sed's stdin pipe was led open by another sed process, so it mattered not that Apache had closed its side of the pipe since the pipe was still open.

OWWW!  That's a case I hadn't considered.  However, the right answer
is to close the children's side of the pipes, always.  If they are left open,
it's much harder to detect that the child has died/you've reached EOF.

>For mod_ext_filter I can hold a thread mutex around the critical section  above, and that protects it from other instances of mod_ext_filter doing fork() and inheriting the wrong handle, but it does nothing for other threads calling apr_proc_create().

The right answer, in part, is to close those child handles.  The second side
is to protect that window where we declare the handles as keep-open on
exec to the point where we've already fork()ed.

>It would seem that all of the pipe creation and child cleanup registration needs to be done inside apr_proc_create() so that it can hold a mutex and all callers of apr_proc_create() will be happy.  I didn't look at it to know if that would require API changes.  I wouldn't be shocked.

For the small window between declaring the handles keep-open,
fork()ing the child and then closing them in the parent, we absolutely
need a mutex to keep things clean.

Otherwise, two children may have the write side of the parent's read pipe,
and when the first (real) child dies, we don't know it.

Bill