You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@perl.apache.org by Geoffrey Young <ge...@modperlcookbook.org> on 2004/01/08 20:45:26 UTC
[todo] server_root_relative
hi all
we have this in our todo/release:
* Apache->server_root_relative:
needs to default to current pool (pconf at startup, r->pool at
request time) - solution: require the pool object to be passed. if a
user doesn't have one, make them create one, e.g.:
Apache::server_root_relative(APR::Pool->new, ....). Must make sure
that the returned SV has a copy of that string and doesn't rely on
anything that it's in pool, which will be now destroyed.
as far as I can tell, this makes Apache::server_root_relative() behave
exactly like ap_server_root_relative() wrt requiring a pool.
so, it looks to me like mod_perl doesn't need to intervene on this call at
anymore, but I wanted to make sure I haven't missed something vital before a
commit. the comments made me nervous, since I couldn't remember if the
discussion at apachecon was that the SV definitely needed to be a copy, or
if it just needed to be checked for integrity - I traced the pool calls back
and didn't see anything that seemed to require the pool stick around, but
I'm not entirely sure I'm looking at it right.
anyway, I added a few tests, including one that destroys the pool then
attempts to reuse the SV. everything passes for me on worker, including
t/SMOKE but I could use another set of eyes.
--Geoff
Re: [todo] server_root_relative
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
> all is now checked in with the above suggestions incorporated.
geoff++
The only missing thing is to grep src/docs/2.0 for server_root_relative and
fix them. thanks.
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [todo] server_root_relative
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
> Beautiful work, Geoff.
thanks. it was much more involved than it appeared to be from looking at
the release file :)
>
> The only comment I have is we should probably use DEFINEs for xs
> wrappers, since your functions don't do anything but pass the args
> through as is.
agreed.
> So if it's not used anywhere else may be it should become static then
> and removed from modperl_util.h?
agreed.
>
>> it no longer returns a global pool at all
>
> I think it's a goodness not to use the global pool for any public APIs
> that could be used at request run-time. Makes it a tiny bit harder on
> users, but at the same time it makes harder for them to shoot themselves
> into their foot.
agreed
>
>> and now supports properly formatted subclasses.
>
> goodness!
:)
all is now checked in with the above suggestions incorporated.
thanks for the feedback.
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [todo] server_root_relative
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
>
> Stas Bekman wrote:
>
>>I think we shouldn't support:
>>
>>Apache->server_root_relative and equivalents, but only:
>>
>>($r|$s|$c)->server_root_relative(...) and
>>Apache::server_root_relative($pool, ...)
>>
>>because users will get bad memory leaks when a global pool is used to
>>allocate memory in requests. We should support the non-(pool|$r|$c|$s)
>>argument functions only in compat
>
>
> ok, the attached patch takes care of this. the only hitch is that
> Apache->server_root_relative in compat.pm collides, so it needed to follow
> the new override/restore API.
Beautiful work, Geoff.
The only comment I have is we should probably use DEFINEs for xs wrappers,
since your functions don't do anything but pass the args through as is.
>>the comments here are confusing. This is a generic internal
>>modperl_sv2pool function and used in other places
>
>
> actually, it's not that I can see. maybe it used to be?
could be.
> anyway, as a result of our new syntax and the fact that sv2pool isn't used
> elsewhere, I've adjusted the function considerably - it no longer returns a
> global pool at all and now supports properly formatted subclasses.
So if it's not used anywhere else may be it should become static then and
removed from modperl_util.h?
> it no longer returns a global pool at all
I think it's a goodness not to use the global pool for any public APIs that
could be used at request run-time. Makes it a tiny bit harder on users, but at
the same time it makes harder for them to shoot themselves into their foot.
> and now supports properly formatted subclasses.
goodness!
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [todo] server_root_relative
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
Stas Bekman wrote:
> I think we shouldn't support:
>
> Apache->server_root_relative and equivalents, but only:
>
> ($r|$s|$c)->server_root_relative(...) and
> Apache::server_root_relative($pool, ...)
>
> because users will get bad memory leaks when a global pool is used to
> allocate memory in requests. We should support the non-(pool|$r|$c|$s)
> argument functions only in compat
ok, the attached patch takes care of this. the only hitch is that
Apache->server_root_relative in compat.pm collides, so it needed to follow
the new override/restore API.
> the comments here are confusing. This is a generic internal
> modperl_sv2pool function and used in other places
actually, it's not that I can see. maybe it used to be?
anyway, as a result of our new syntax and the fact that sv2pool isn't used
elsewhere, I've adjusted the function considerably - it no longer returns a
global pool at all and now supports properly formatted subclasses.
--Geoff
Re: [todo] server_root_relative
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
>>Besides comments below, I think we shouldn't support:
>>
>>Apache->server_root_relative and equivalents, but only:
>>
>>($r|$s|$c)->server_root_relative(...) and
>>Apache::server_root_relative($pool, ...)
>>
>>because users will get bad memory leaks when a global pool is used to
>>allocate memory in requests. We should support the non-(pool|$r|$c|$s)
>>argument functions only in compat and may be even print a warning when
>>used for the first time.
>>
>>What do you think?
>
>
> sounds fine to me. the functional no-args for was bothersome to me anyway,
> and this makes coding it easier.
great!
>>I don't think the non-Server functions belong here. They should go into
>>respective xs/Apache/ files.
>
>
> I did try that, but I couldn't seem to get to mpxs_ap_server_root_relative
> from the other .h files. but then I thought it was ok - there are lots of
> examples where .h files are used to define methods for other classes,
> especially the util classes. for instance Apache::server_root_relative
> lives in Apache__ServerUtil.h, Apache::RequestRec::as_string lives in
> Apache__RequestUtil.h, etc.
Yes, but but notice that as_string is still operating on /Request/, whereas
your patch was putting request and connection methods into a /Server/. Not good.
I'm not sure about the include to reuse the definition, but it's really a
one-liner, which can be #defined too:
#define mpxs_ap_server_root_relative(sv, fname) \
newSVpv(ap_server_root_relative(modperl_sv2pool(aTHX_ sv), fname), 0);
so may be add it to xs/modperl_xs_util.h, which is alredy included.
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [todo] server_root_relative
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
> Besides comments below, I think we shouldn't support:
>
> Apache->server_root_relative and equivalents, but only:
>
> ($r|$s|$c)->server_root_relative(...) and
> Apache::server_root_relative($pool, ...)
>
> because users will get bad memory leaks when a global pool is used to
> allocate memory in requests. We should support the non-(pool|$r|$c|$s)
> argument functions only in compat and may be even print a warning when
> used for the first time.
>
> What do you think?
sounds fine to me. the functional no-args for was bothersome to me anyway,
and this makes coding it easier.
> I don't think the non-Server functions belong here. They should go into
> respective xs/Apache/ files.
I did try that, but I couldn't seem to get to mpxs_ap_server_root_relative
from the other .h files. but then I thought it was ok - there are lots of
examples where .h files are used to define methods for other classes,
especially the util classes. for instance Apache::server_root_relative
lives in Apache__ServerUtil.h, Apache::RequestRec::as_string lives in
Apache__RequestUtil.h, etc.
so there's precedent. is it ok this way?
> Can't #define be used here, they are all
> the same?
#define would probably be better. I'll do that instead.
thanks :)
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [todo] server_root_relative
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
> ok, here's another pass - I think it covers what we talked about this
> afternoon on irc.
>
> it won't apply over the is_perl_option_enabled stuff you just posted, so
> I'll wait until monday to rework it into core and commit, even if there are
> no technical changes to be made.
>
> if you have any ideas on a better way to phrase Changes, I'm all ears.
Besides comments below, I think we shouldn't support:
Apache->server_root_relative and equivalents, but only:
($r|$s|$c)->server_root_relative(...) and Apache::server_root_relative($pool, ...)
because users will get bad memory leaks when a global pool is used to allocate
memory in requests. We should support the non-(pool|$r|$c|$s) argument
functions only in compat and may be even print a warning when used for the
first time.
What do you think?
> Index: Changes
> ===================================================================
> RCS file: /home/cvspublic/modperl-2.0/Changes,v
> retrieving revision 1.297
> diff -u -r1.297 Changes
> --- Changes 3 Jan 2004 01:17:33 -0000 1.297
> +++ Changes 10 Jan 2004 02:46:07 -0000
> @@ -11,6 +11,8 @@
> =over 3
>
> =item 1.99_13-dev
> +server_root_relative() will now derive the pool if called from
> +$r, $s, or $c objects. [Geoffrey Young]
How about:
server_root_relative() now derives the pool if called from
$r, $s, or $c objects and copies the result so it can be used even if the pool
used to allocate the result went out of scope. [Geoffrey Young]
> On Solaris add a workaround for xs/APR/APR/Makefile.PL to build
> APR.so, correctly linked against apr and apr-util libs, by addding the
> Index: src/modules/perl/modperl_util.c
> ===================================================================
> RCS file: /home/cvspublic/modperl-2.0/src/modules/perl/modperl_util.c,v
> retrieving revision 1.59
> diff -u -r1.59 modperl_util.c
> --- src/modules/perl/modperl_util.c 19 Dec 2003 01:17:32 -0000 1.59
> +++ src/modules/perl/modperl_util.c 10 Jan 2004 02:46:07 -0000
> @@ -194,6 +194,12 @@
> return modperl_global_get_pconf();
> }
the comments here are confusing. This is a generic internal modperl_sv2pool
function and used in other places, so I think only the second para of the
comment is relevant. Also it's not Apache->server_root_relative($p, $name),
but Apache::server_root_relative($p, $name)
> + /* $r->server_root_relative($name), or
> + * Apache->server_root_relative($p, $name)
> + *
> + * for $(r|c|s) dig out the pool from the record
> + * otherwise use the pool provided
> + */
> if ((SvROK(obj) && (SvTYPE(SvRV(obj)) == SVt_PVMG))) {
> ptr = SvObjIV(obj);
> classname = SvCLASS(obj);
> @@ -204,8 +210,13 @@
> }
>
> if (*classname != 'A') {
> - /* XXX: could be a subclass */
> - return NULL;
> + /* XXX: could be a subclass, but more likely
> + * Apache::server_root_relative($name);
> + * note no -> so $name is the class
> + * avoid core dumps by returning the global pool,
> + * even though it then gives just the ServerRoot back
> + */
> + return modperl_global_get_pconf();
> }
again, this function is not used solely by s.r.r. returning a global pool
could be a very bad guess, causing bad memory leaks. I'd keep that NULL in
place and handle the checking for subclass in another iteration, but
definitely not returning a global pool.
> Index: xs/Apache/ServerUtil/Apache__ServerUtil.h
> ===================================================================
> RCS file: /home/cvspublic/modperl-2.0/xs/Apache/ServerUtil/Apache__ServerUtil.h,v
> retrieving revision 1.8
> diff -u -r1.8 Apache__ServerUtil.h
> --- xs/Apache/ServerUtil/Apache__ServerUtil.h 19 Nov 2001 23:46:48 -0000 1.8
> +++ xs/Apache/ServerUtil/Apache__ServerUtil.h 10 Jan 2004 02:46:07 -0000
> @@ -42,13 +42,37 @@
> #define mpxs_Apache_server(classname) \
> modperl_global_get_server_rec()
> -static MP_INLINE char *mpxs_ap_server_root_relative(pTHX_
> - SV *sv,
> - const char *fname)
> +static MP_INLINE SV *mpxs_ap_server_root_relative(pTHX_
> + SV *sv,
> + const char *fname)
> {
> apr_pool_t *p = modperl_sv2pool(aTHX_ sv);
>
> - return ap_server_root_relative(p, fname);
please add a comment of why do you do that, e.g.:
/* make a copy of the result to avoid situations where the pool goes
* out of scope and the sv containing a pointer to a freed memory is
* attempted to be used
*/
> + return newSVpv(ap_server_root_relative(p, fname), 0);
> +}
I don't think the non-Server functions belong here. They should go into
respective xs/Apache/ files. Can't #define be used here, they are all the same?
> +static MP_INLINE
> +SV *mpxs_Apache__RequestRec_server_root_relative(pTHX_
> + SV *sv,
> + const char *fname)
> +{
> + return mpxs_ap_server_root_relative(aTHX_ sv, fname);
> +}
> +
> +static MP_INLINE
> +SV *mpxs_Apache__Server_server_root_relative(pTHX_
> + SV *sv,
> + const char *fname)
> +{
> + return mpxs_ap_server_root_relative(aTHX_ sv, fname);
> +}
> +
> +static MP_INLINE
> +SV *mpxs_Apache__Connection_server_root_relative(pTHX_
> + SV *sv,
> + const char *fname)
> +{
> + return mpxs_ap_server_root_relative(aTHX_ sv, fname);
> }
>
> static void mpxs_Apache__ServerUtil_BOOT(pTHX)
> Index: xs/maps/modperl_functions.map
> ===================================================================
> RCS file: /home/cvspublic/modperl-2.0/xs/maps/modperl_functions.map,v
> retrieving revision 1.63
> diff -u -r1.63 modperl_functions.map
> --- xs/maps/modperl_functions.map 23 Dec 2003 03:02:34 -0000 1.63
> +++ xs/maps/modperl_functions.map 10 Jan 2004 02:46:07 -0000
> @@ -69,8 +69,15 @@
> mpxs_Apache__Server_get_handlers
> modperl_config_insert_server | | | add_config
>
> +PACKAGE=Apache::RequestRec
> + mpxs_Apache__RequestRec_server_root_relative | | SV *:p, fname=""
> +
> PACKAGE=Apache::Server
> SV *:DEFINE_dir_config | | server_rec *:s, char *:key=NULL, SV *:sv_val=Nullsv
> + mpxs_Apache__Server_server_root_relative | | SV *:p, fname=""
> +
> +PACKAGE=Apache::Connection
> + mpxs_Apache__Connection_server_root_relative | | SV *:p, fname=""
>
> PACKAGE=Apache
> server_rec *:DEFINE_server | | SV *:classname=Nullsv
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [todo] server_root_relative
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
> it won't apply over the is_perl_option_enabled stuff you just posted, so
> I'll wait until monday to rework it into core and commit, even if there are
> no technical changes to be made.
wow, that was a fast commit :)
anyway, here's a patch against current cvs for you to review when you have
time - I'm off for the weekend, so I'll still only get to it monday.
--Geoff
Re: [todo] server_root_relative
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
ok, here's another pass - I think it covers what we talked about this
afternoon on irc.
it won't apply over the is_perl_option_enabled stuff you just posted, so
I'll wait until monday to rework it into core and commit, even if there are
no technical changes to be made.
if you have any ideas on a better way to phrase Changes, I'm all ears.
--Geoff
Re: [todo] server_root_relative
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
> well, I think originally doug who has written that entry meant that
> server_root_relative should be able to figure out which pool to use on
> its own. I could be wrong. Your proposed change makes the API more
> complex (though it now maps 1:1 to the C API) and incompatible with mp1.
well, I thought it was the API we had agreed upon in vegas :)
>
> What do you think about the following: keep things as they are now -
> i.e. you can call $r->server_root_relative(),
> $c->server_root_relative(), etc. but if you call
> Apache::server_root_relative() it'll just use APR::Pool->new (in C of
> course) behind the scenes?
hmm, ok, let me think about that.
> We discussed several times about having a
> state variable/function which can tell us which phase we are in, so
> later on if we implement it we could assist ourselves using it and for
> example pick pconf if we figure that we are in one of the startup phases.
ugh.
>> anyway, I added a few tests, including one that destroys the pool then
>> attempts to reuse the SV. everything passes for me on worker, including
>> t/SMOKE but I could use another set of eyes.
>
>
> I think it just happens to work for you, since the freed memory wasn't
> overwritten by somebody else.
yeah, that was what I was afraid of.
> Notice that we need to ensure the copy only if use APR::Pool->new, with
> any other pool it's the responsibility of the user not to use that
> variable after the pool is gone. Oh, may be it's a bad assumption and we
> should always copy it. The biggest problem with not copying is that
> users will have hard time tracking this issue down, if they use a
> variable after the pool has gone out of scope. Sometimes it will work
> (if the freed memory is still free), sometimes it won't (and crash).
ok, it's easy enough to return a copy. but lemme give it some more thought.
I really don't mind making it a class method only and passing in a pool,
though :)
thanks for the feedback.
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [todo] server_root_relative
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
> hi all
>
> we have this in our todo/release:
>
> * Apache->server_root_relative:
> needs to default to current pool (pconf at startup, r->pool at
> request time) - solution: require the pool object to be passed. if a
> user doesn't have one, make them create one, e.g.:
> Apache::server_root_relative(APR::Pool->new, ....). Must make sure
> that the returned SV has a copy of that string and doesn't rely on
> anything that it's in pool, which will be now destroyed.
>
> as far as I can tell, this makes Apache::server_root_relative() behave
> exactly like ap_server_root_relative() wrt requiring a pool.
well, I think originally doug who has written that entry meant that
server_root_relative should be able to figure out which pool to use on its
own. I could be wrong. Your proposed change makes the API more complex (though
it now maps 1:1 to the C API) and incompatible with mp1.
What do you think about the following: keep things as they are now - i.e. you
can call $r->server_root_relative(), $c->server_root_relative(), etc. but if
you call Apache::server_root_relative() it'll just use APR::Pool->new (in C of
course) behind the scenes? We discussed several times about having a state
variable/function which can tell us which phase we are in, so later on if we
implement it we could assist ourselves using it and for example pick pconf if
we figure that we are in one of the startup phases.
> so, it looks to me like mod_perl doesn't need to intervene on this call at
> anymore, but I wanted to make sure I haven't missed something vital before a
> commit. the comments made me nervous, since I couldn't remember if the
> discussion at apachecon was that the SV definitely needed to be a copy, or
> if it just needed to be checked for integrity - I traced the pool calls back
> and didn't see anything that seemed to require the pool stick around, but
> I'm not entirely sure I'm looking at it right.
>
> anyway, I added a few tests, including one that destroys the pool then
> attempts to reuse the SV. everything passes for me on worker, including
> t/SMOKE but I could use another set of eyes.
I think it just happens to work for you, since the freed memory wasn't
overwritten by somebody else. It doesn't make sense if it works without making
a copy of the string. If the pool goes away all its memory goes invalid, if
you have a pointer to that memory, it now points to invalid memory (which may
still contain seemingly good data). I'd add a debug print of the pointer when
the string is allocated and then dump the SV to see if it's the same pointer,
or simply look at the code to see whether it gets copied somewhere.
Notice that we need to ensure the copy only if use APR::Pool->new, with any
other pool it's the responsibility of the user not to use that variable after
the pool is gone. Oh, may be it's a bad assumption and we should always copy
it. The biggest problem with not copying is that users will have hard time
tracking this issue down, if they use a variable after the pool has gone out
of scope. Sometimes it will work (if the freed memory is still free),
sometimes it won't (and crash).
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org