You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Aaron Bannert <aa...@ebuilt.com> on 2001/07/18 09:04:43 UTC
[PATCH] Fix apr_thread_exit(), change apr_thread_start_t, etc...
This patch provides a few fixes and a couple changes to the APR
threads API as discussed on this list. This does not implement the
controversial issues from the list, but merly a couple bug fixes I've
found and a widening of the worker_fn() parameters to make it possible
for applications to call functions like apr_thread_exit() and actually
use the childpool that is implicitly created.
In all cases they were tested on Unix (using my Linux 2.4 box). I
attempted to get the code for beos, os2, and win32 as close to
what I think it should be, but others will have to verify my changes:
1) Fix to apr_thread_join() -- it wasn't setting the return value correctly.
2) Added a new test in test/testthread.c to verify that the above works.
3) Changed the prototype for apr_thread_start_t to pass in the pool context
and the apr_thread_t structure for a couple reasons:
a) apr_thread_exit() requires the apr_thread_t, this makes it explicit
to the application developer where that comes from.
b) apr_thread_create() was creating a child-pool but only saving that
child pool in the private platform-specific apr_thread_t struct
definition, making it unavailable to the actual thread. Now it is
directly available and the app programmer knows exactly from which
pool to do allocations/cleanup registrations.
Like I said, I tested this on my Linux box, and did my best on the rest.
Win32 was the most unique implementation, so it would be best if someone
could run the new test/testthread to see if that was even working before
and that it works now (it was broken on unix and beos, os2 was ok IIRC).
enjoy,
-aaron
Re: [PATCH] Fix apr_thread_exit(), change apr_thread_start_t, etc...
Posted by rb...@covalent.net.
On Wed, 18 Jul 2001, Justin Erenkrantz wrote:
> On Wed, Jul 18, 2001 at 08:18:25AM -0700, rbb@covalent.net wrote:
> > IMNSHO, this is wrong for two reasons. #1, if the app has access to the
> > apr_thread_t, then they also have access to the pool. We have a macro
> > called APR_POOL_IMPLEMENT_ACCESSOR that specifically creates accessor
> > functions for the pools. Threads may not implement that function yet, but
> > it is far better to allow one method for accessing the pool. #2, why are
> > we passing the apr_thread_t as a separate parameter to the thread? That
> > is introducing a layer of indirection that isn't required here. Just
> > create an apr_thread_param_t structure that is:
>
> I believe that we don't have access to the pool in apr_thread_t. This
> is a private structure that is defined per architecture. If there is a
> way of accessing the pools in the apr_thread_t portably, I'd like to
> know where it is. The obvious thing to do is look in the struct, but we
> can't do that. Please enlighten us by showing us where in the code to
> do this. =)
Re-read my message please. We have a macro APR_POOL_IMPLEMENT_ACCESSOR,
which will create a function that gives you access to the pool from ANY
apr type. Just grep the APR code to determine how to use it.
> As for passing the apr_thread_t in, we need it for apr_thread_exit for
> reasons described earlier. For example, in the threaded MPM, the
> apr_thread_t** exists only on the stack not in static space. I think it
> would be cleaner if we passed it in to the worker function rather than
> relying on it being in a global scope.
Again, re-read my message. I didn't say don't pass it to the thread
function. I said don't do it the way your patch implements it.
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------
Re: [PATCH] Fix apr_thread_exit(), change apr_thread_start_t, etc...
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Wed, Jul 18, 2001 at 08:18:25AM -0700, rbb@covalent.net wrote:
> IMNSHO, this is wrong for two reasons. #1, if the app has access to the
> apr_thread_t, then they also have access to the pool. We have a macro
> called APR_POOL_IMPLEMENT_ACCESSOR that specifically creates accessor
> functions for the pools. Threads may not implement that function yet, but
> it is far better to allow one method for accessing the pool. #2, why are
> we passing the apr_thread_t as a separate parameter to the thread? That
> is introducing a layer of indirection that isn't required here. Just
> create an apr_thread_param_t structure that is:
I believe that we don't have access to the pool in apr_thread_t. This
is a private structure that is defined per architecture. If there is a
way of accessing the pools in the apr_thread_t portably, I'd like to
know where it is. The obvious thing to do is look in the struct, but we
can't do that. Please enlighten us by showing us where in the code to
do this. =)
As for passing the apr_thread_t in, we need it for apr_thread_exit for
reasons described earlier. For example, in the threaded MPM, the
apr_thread_t** exists only on the stack not in static space. I think it
would be cleaner if we passed it in to the worker function rather than
relying on it being in a global scope.
> Now, we can re-define the APR_THREAD_FUNC api to be
>
> +typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_param_t *);
Fine. I don't care much, but I think the other way is a tad more
explicit/clearer about what to do. It all depends on where the
marshaling/unmarshaling happens. It achieves the same result, IMHO.
-- justin
Re: [PATCH] Fix apr_thread_exit(), change apr_thread_start_t, etc...
Posted by rb...@covalent.net.
> > IMNSHO, this is wrong for two reasons. #1, if the app has access to the
> > apr_thread_t, then they also have access to the pool.
>
> For the app to get access to the apr_thread_t, it must pass it through
> in it's own opaque data pointer. This is of course totally unobvious to
> the application developer.
>
>
> > We have a macro
> > called APR_POOL_IMPLEMENT_ACCESSOR that specifically creates accessor
> > functions for the pools. Threads may not implement that function yet, but
> > it is far better to allow one method for accessing the pool.
>
> Threads may not implement that function yet because apr_thread_t is
> a private os-dependent data structure. Now an application developer
> must not only *always* pass the apr_thread_t through it's opaque data
> structure (in addition to whatever other data they need to pass), but
> once they are done doing that they're going to have to call some
> macro to extract the pool from the apr_thread_t? Ignoring the added
> complexity and obfuscation from an API user's standpoint just for a
> moment, and I still fail to see how this is portable.
you aren't looking at the code. File's are implemented on a per platform
basis, so that unix and windows files share nothing when it comes to the
file type definition. However, we have already implemented the pool
accessor functions using the macros I have pointed out.
As for the API, allow me to show what I am asking for
apr_thread_func thread_func(apr_thread_param_t *param)
{
apr_thread_t *t = param->t;
worker_data = param->data;
apr_pool_t *p = apr_thread_get_pool(t);
.....
}
> [Side note: In order to use APR_POOL_IMPLEMENT_ACCESSOR, will apr_thread_t
> have to be a non-os-dependent structure; one that contains the current
> os-defined private structure as a member? I see this as an alternative
> solution analog to the APR_POOL_IMPLEMENT_ACCESSOR routine, but I reject
> the idea that this would be a "good clean API".]
Please read the code.
> Every other function in APR that would need immediate access to a pool
> is passed explicity in the parameter list.
That is just not true. Every APR function has access to a pool. It gets
access to the pool in one of two ways. 1) it is passed in through the
parameter list. 2) It is passed in through some other variable on the
parameter list. So, if a file function needs access to a pool, then if an
apr_file_t is passed in, then that function has access to the pool through
the file_t and generally another pool is not passed in. This is not
always true. If we are trying to change the scope of a file, then we may
pass in a pool to a file function, even if there is already a pool passed
in through another parameter.
> > #2, why are
> > we passing the apr_thread_t as a separate parameter to the thread? That
> > is introducing a layer of indirection that isn't required here. Just
> > create an apr_thread_param_t structure that is:
> >
> > struct apr_thread_param_t {
> > apr_thread_t *t;
> > void *data;
> > }
> >
> > Then, we can pass in this value as the void * to the thread function, and
> > be done with it. This allows us to call the function passed into
> > apr_thread_create directly, and it allows us to add more parameters to
> > thread creation later if we have to. I am not saying that we will
> > definately have to, but at least this allows us to do so easily.
> >
> > Now, we can re-define the APR_THREAD_FUNC api to be
> >
> > +typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_param_t *);
> >
>
> So I'm willing to repost the patch (inline this time :) with the
> apr_thread_param_t modification suggested above, but I'm not willing
> to leave the apr_thread_t as a necessary but missing parameter, nor
> am I willing to leave the pool off the parameter list if we can't
> get to it easily and portably.
We can get to all of them easily and portably.
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------
Re: [PATCH] Fix apr_thread_exit(), change apr_thread_start_t, etc...
Posted by Aaron Bannert <aa...@ebuilt.com>.
On Wed, Jul 18, 2001 at 08:18:25AM -0700, rbb@covalent.net wrote:
>
> -1. Please post patches in-line, because it makes it MUCH easier to reply
> to them and make useful comments. Also, posting one patch at a time
> would have made this patch easier to follow, and it would have
> allowed me to commit the pieces that I like without having to go in
> and modify the patch itself. Having said that, my comments follow.
No prob, next time it'll be inline.
>
> Index: include/apr_thread_proc.h
> ===================================================================
> RCS file: /home/cvspublic/apr/include/apr_thread_proc.h,v
> retrieving revision 1.65
> diff -u -r1.65 apr_thread_proc.h
> --- include/apr_thread_proc.h 2001/06/06 18:11:06 1.65
> +++ include/apr_thread_proc.h 2001/07/18 06:27:04
> @@ -125,7 +125,7 @@
> typedef struct apr_other_child_rec_t apr_other_child_rec_t;
> #endif /* APR_HAS_OTHER_CHILD */
>
> -typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(void *);
> +typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(void *,
> apr_thread_t *, apr_pool_t *);
>
> IMNSHO, this is wrong for two reasons. #1, if the app has access to the
> apr_thread_t, then they also have access to the pool.
For the app to get access to the apr_thread_t, it must pass it through
in it's own opaque data pointer. This is of course totally unobvious to
the application developer.
> We have a macro
> called APR_POOL_IMPLEMENT_ACCESSOR that specifically creates accessor
> functions for the pools. Threads may not implement that function yet, but
> it is far better to allow one method for accessing the pool.
Threads may not implement that function yet because apr_thread_t is
a private os-dependent data structure. Now an application developer
must not only *always* pass the apr_thread_t through it's opaque data
structure (in addition to whatever other data they need to pass), but
once they are done doing that they're going to have to call some
macro to extract the pool from the apr_thread_t? Ignoring the added
complexity and obfuscation from an API user's standpoint just for a
moment, and I still fail to see how this is portable.
[Side note: In order to use APR_POOL_IMPLEMENT_ACCESSOR, will apr_thread_t
have to be a non-os-dependent structure; one that contains the current
os-defined private structure as a member? I see this as an alternative
solution analog to the APR_POOL_IMPLEMENT_ACCESSOR routine, but I reject
the idea that this would be a "good clean API".]
Every other function in APR that would need immediate access to a pool
is passed explicity in the parameter list.
> #2, why are
> we passing the apr_thread_t as a separate parameter to the thread? That
> is introducing a layer of indirection that isn't required here. Just
> create an apr_thread_param_t structure that is:
>
> struct apr_thread_param_t {
> apr_thread_t *t;
> void *data;
> }
>
> Then, we can pass in this value as the void * to the thread function, and
> be done with it. This allows us to call the function passed into
> apr_thread_create directly, and it allows us to add more parameters to
> thread creation later if we have to. I am not saying that we will
> definately have to, but at least this allows us to do so easily.
>
> Now, we can re-define the APR_THREAD_FUNC api to be
>
> +typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_param_t *);
>
Either way is fine for me as long as it's really obvious to the app developer
where to get the apr_thread_t AND the apr_pool_t from. I also agree that
the "unbundled" method you describe here would be easier to maintain in
the case that we'd want to add more parameters later.
So I'm willing to repost the patch (inline this time :) with the
apr_thread_param_t modification suggested above, but I'm not willing
to leave the apr_thread_t as a necessary but missing parameter, nor
am I willing to leave the pool off the parameter list if we can't
get to it easily and portably.
-aaron
Re: [PATCH] Fix apr_thread_exit(), change apr_thread_start_t, etc...
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: <rb...@covalent.net>
Cc: <de...@apr.apache.org>
Subject: Re: [PATCH] Fix apr_thread_exit(), change apr_thread_start_t, etc...
> Please post patches in-line, because it makes it MUCH easier to reply
> to them and make useful comments.
I believe the rule is simply attach in text/plain. Some mail agents are for
shit (outlook express, for one :) and I don't want to have to start mucking with
wrapped patches :( If it works inline for you, great, if not...
> Also, posting one patch at a time would have made this patch easier to follow,
> and it would have allowed me to commit the pieces that I like
+1
Re: [PATCH] Fix apr_thread_exit(), change apr_thread_start_t, etc...
Posted by rb...@covalent.net.
-1. Please post patches in-line, because it makes it MUCH easier to reply
to them and make useful comments. Also, posting one patch at a time
would have made this patch easier to follow, and it would have
allowed me to commit the pieces that I like without having to go in
and modify the patch itself. Having said that, my comments follow.
Index: include/apr_thread_proc.h
===================================================================
RCS file: /home/cvspublic/apr/include/apr_thread_proc.h,v
retrieving revision 1.65
diff -u -r1.65 apr_thread_proc.h
--- include/apr_thread_proc.h 2001/06/06 18:11:06 1.65
+++ include/apr_thread_proc.h 2001/07/18 06:27:04
@@ -125,7 +125,7 @@
typedef struct apr_other_child_rec_t apr_other_child_rec_t;
#endif /* APR_HAS_OTHER_CHILD */
-typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(void *);
+typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(void *,
apr_thread_t *, apr_pool_t *);
IMNSHO, this is wrong for two reasons. #1, if the app has access to the
apr_thread_t, then they also have access to the pool. We have a macro
called APR_POOL_IMPLEMENT_ACCESSOR that specifically creates accessor
functions for the pools. Threads may not implement that function yet, but
it is far better to allow one method for accessing the pool. #2, why are
we passing the apr_thread_t as a separate parameter to the thread? That
is introducing a layer of indirection that isn't required here. Just
create an apr_thread_param_t structure that is:
struct apr_thread_param_t {
apr_thread_t *t;
void *data;
}
Then, we can pass in this value as the void * to the thread function, and
be done with it. This allows us to call the function passed into
apr_thread_create directly, and it allows us to add more parameters to
thread creation later if we have to. I am not saying that we will
definately have to, but at least this allows us to do so easily.
Now, we can re-define the APR_THREAD_FUNC api to be
+typedef void *(APR_THREAD_FUNC *apr_thread_start_t)(apr_thread_param_t *);
Done.
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------