[PATCH] Automatic module globals management

Steph

20 years ago

Hey Dmitry, all,

Disclaimer: I've had a sneak preview on this one.

I really don't like this going into 5_2 branch, and the more I think about
it the less sure I am about it going into HEAD either. It would mean
renaming all the ctor/dtor functions to the new globals, which is going to
take an awful lot of #ifdef'ing in PECL extensions if they're going to
retain compatibility across all the current versions of PHP.

I've spent some time fiddling with this stuff as a result.

In ZTS builds the dtor functions are actually called correctly, at the point
when the resource is freed. The original ZTS problem (of how to store the
TSRM resource ID in the zend_module_entry struct) is resolvable without
going to all those lengths - I have it running locally and can provide a
patch when I clean up my source - but would still involve a change to the
module API, because that's unavoidable. Just a far less radical change from
the perspective of extension authors.

In non-ZTS builds the dtor functions are only called from MSHUTDOWN, and
that's a separate issue altogether. I've been toying with the idea of
storing a pointer to the dtor (where one exists) in a hashtable keyed on
module name and calling it from the module destructor. I've got as far as
the module destructor recognising when there's a dtor declared in
ZEND_INIT_MODULE_GLOBALS, but the next part's kind of eluding me at present.
If you're interested I'll pull together what I have so far and send it over
to the experts :)

The 'down' side is that I don't know how much storing extension dtor info in
this way would impact startup speed, and I've no way to check. But then
again I don't know how storing it in the module struct would impact startup
speed either...

- Steph

----- Original Message -----
From: "Dmitry Stogov" <dmitry@zend.com>
To: <internals@lists.php.net>; "Ilia Alshanetsky" <ilia@prohost.org>
Cc: "Andi Gutmans" <andi@zend.com>; "Steph" <steph@zend.com>
Sent: Thursday, June 08, 2006 2:15 PM
Subject: [PHP-DEV] [PATCH] Automatic module globals management

> Hi,
>
> The attached patch (for PHP_5_2) implements automatic management of module
> globals.
> The problem that module globals must be unregistered before extension
> unloading, because "globls_dtor" function is unloaded together with
> extension and cannot be called.
>
> To solve this problem extension writers now use the following pattern:
>
> PHP_MSHUTDOWN_FUNCTION(mod_name)
> {
> -#ifdef ZTS
> - ts_free_id(mod_name_globals_id);
> -#else
> - mod_name_globals_dtor(&mod_name_globals TSRMLS_CC);
> -#endif
>
> With my patch, extension writers should just extend module descriptor with
> globals descriptor and ctor/dtor callbacks.
>
>  PHP_RSHUTDOWN(mod_name),
>  PHP_MINFO(mod_name),
>  NO_VERSION_YET,
> - STANDARD_MODULE_PROPERTIES
> + NULL,
> + ZEND_MG(mod_name),
> + ZEND_MGCTOR(mod_name),
> + ZEND_MGCTOR(mod_name),
> + STANDARD_MODULE_PROPERTIES_EX2
> };
>
> Old extensions are source compatible and may work without modification.
> The patch modifies only several extensions, but will modify others too.
>
> I like commit the patch into HEAD and PHP_5_2.
> Any objections, additional ideas?
>
> Thanks. Dmitry.
>
>
> __________ NOD32 1.1380 (20060125) Information __________
>
> This message was checked by NOD32 antivirus system.
> http://www.eset.com
>
>

--------------------------------------------------------------------------------

Sara Golemon

20 years ago

> The attached patch (for PHP_5_2) implements automatic management of module
> globals.
> The problem that module globals must be unregistered before extension
> unloading, because "globls_dtor" function is unloaded together with
> extension and cannot be called.
>

Dmitry-

I like the concept especially as it makes this part of the lifecycle-loop
consistent with the RINIT/SHUTDOWN MINIT/SHUTDOWN design....

Having said that I like it, here's what I'd change:

*) Nomenclature:  I'd like to see MGCTOR/MGDTOR (and their proto wrappers)
named more consistently with [RM]INIT/SHUTDOWN. e.g.   GINIT/GSHUTDOWN  or
TINIT/TSHUTDOWN

*) There are some WS problems in your patch to posix.c

> I really don't like this going into 5_2 branch, and
> the more I think about it the less sure I am about
> it going into HEAD either. It would mean renaming
> all the ctor/dtor functions to the new globals, which
> is going to take an awful lot of #ifdef'ing in PECL
> extensions if they're going to retain compatibility
> across all the current versions of PHP.
>

Steph-

I think you're missing the fact that Dmitry's patch only *enables* an
extension to register CTOR/DTORs in the module entry struct.  Existing
modules can continue to use the manual calls in MINIT/MSHUTDOWN without
breaking.

-Sara

Steph

20 years ago

Hi Sara,

>  I think you're missing the fact that Dmitry's patch only *enables* an
> extension to register CTOR/DTORs in the module entry struct.  Existing
> modules can continue to use the manual calls in MINIT/MSHUTDOWN without
> breaking.

Yes, I know. But altered modules won't be able to use older versions of PHP
readily, and that's going to be a real problem in PECL - especially if this
goes into 5_2 branch.

- Steph

Sara Golemon

20 years ago

> Yes, I know. But altered modules won't be able to use older versions of
> PHP readily, and that's going to be a real problem in PECL - especially if
> this goes into 5_2 branch.
>

For the modules which are part of the core distribution this is a non-issue
as they're branched.  For PECL modules this is the same "how old of a PHP
version am I (as the package maintainer) willing to support?".  That package
maintainer can: (A) Not use the new API, (B) Wrap up some #ifdef's, or (C)
Use the new API exclusively and eschew pre-5.2.

Personally, I'm not going to actually use the GINIT/GSHUTDOWN methods in
extensions like runkit (probably ever) as they're enough of a problem
maintaining PHP 4.0 compatability as it is.  For exts that already require
5.0+, I probably will fold in the use of these after 6.0's release (at which
point 5.2 will be established and in {relatively} wide use).

Allowing the engine to exprt this functionality doesn't automatically break
anything, but when modules *do* switch to the new API, having support for it
going back to 5.2 will be *better* than only having the support go back to
6.0.

-Sara

Steph

20 years ago

>> Yes, I know. But altered modules won't be able to use older versions of
>> PHP readily, and that's going to be a real problem in PECL - especially
>> if this goes into 5_2 branch.
>>
> For the modules which are part of the core distribution this is a
> non-issue as they're branched.  For PECL modules this is the same "how old
> of a PHP version am I (as the package maintainer) willing to support?".
> That package maintainer can: (A) Not use the new API, (B) Wrap up some
> #ifdef's, or (C) Use the new API exclusively and eschew pre-5.2.

Right, and that's why I think it's problematic to have such a major change
in the API, because (A) and (C) are the likeliest outcomes - (B) is
virtually unmanageable. Take a look at what it takes for an extension to
adapt to this, and then imagine all of it #ifdef'd....

- Steph

Sara Golemon

20 years ago

>> For the modules which are part of the core distribution this is a
>> non-issue as they're branched.  For PECL modules this is the same "how
>> old of a PHP version am I (as the package maintainer) willing to
>> support?". That package maintainer can: (A) Not use the new API, (B) Wrap
>> up some #ifdef's, or (C) Use the new API exclusively and eschew pre-5.2.
>
> Right, and that's why I think it's problematic to have such a major change
> in the API, because (A) and (C) are the likeliest outcomes - (B) is
> virtually unmanageable. Take a look at what it takes for an extension to
> adapt to this, and then imagine all of it #ifdef'd....
>

The important part of (A) is "not yet" (which I tried to express in my
second paragraph).  It's the same reason why userspace package maintainers
aren't using exceptions and other 5.x specific features en masse *yet*, and
why goto won't be used in many scripts when 6.0 is first out -- okay, not
the only reason why it won't be used :)

As for (C), I can't imagine an extension maintainer going that route unless
they have other *more compelling* reasons to give up on pre-5.2 versions.
There's just not enough of a win to merit alienating those earlier versions
*yet*.  Noone's forcing module authors to break compatability with older
versions, this is just giving them the choice to do so, and get a cleaner
piece of source in the process.  It's also making it so that when those
extension maintainers do reach that point, they don't have to alienate the
(presumably larger) pre-6.0 audience, they only have to do so for the
(smaller) pre-5.2 audience.

I'm sorry Steph, I know your position, but I just don't understand it.  This
is a minor gain at no cost.  Minor enough that it's not worth it to me to
fight with you over it, but no-cost enough to compel me to against my better
judgement... :)

-Sara

Steph

20 years ago

> I'm sorry Steph, I know your position, but I just don't understand it.
> This is a minor gain at no cost.  Minor enough that it's not worth it to
> me to fight with you over it, but no-cost enough to compel me to against
> my better judgement... :)

That's because you're missing that this was originally offered as a FIX....

Stanislav Malyshev

20 years ago

DS>>The attached patch (for PHP_5_2) implements automatic management of module
DS>>globals.
DS>>The problem that module globals must be unregistered before extension
DS>>unloading, because "globls_dtor" function is unloaded together with
DS>>extension and cannot be called.

I think the patch itself looks fine, and makes globals management cleaner.
However, I'm not sure it would be for 5.2 for reasons cited by Steph -
probably not a lot of extension would be reworked specifically for 5.2
(correct me if I'm wrong here) but for 6.0 probably most of them would
need some massaging here and there, so putting it in 6.0 is quite OK, but
putting it into 5.2 probably would not be used too much outside the engine
modules - for which it doesn't matter much since we can fix them with old
API as far as I see.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/ +972-3-6139665 ext.115

Steph

20 years ago

> DS>>The attached patch (for PHP_5_2) implements automatic management of
> module
> DS>>globals.
> DS>>The problem that module globals must be unregistered before extension
> DS>>unloading, because "globls_dtor" function is unloaded together with
> DS>>extension and cannot be called.
>
> I think the patch itself looks fine, and makes globals management cleaner.
> However, I'm not sure it would be for 5.2 for reasons cited by Steph -
> probably not a lot of extension would be reworked specifically for 5.2
> (correct me if I'm wrong here) but for 6.0 probably most of them would
> need some massaging here and there, so putting it in 6.0 is quite OK, but
> putting it into 5.2 probably would not be used too much outside the engine
> modules - for which it doesn't matter much since we can fix them with old
> API as far as I see.

We can, but Dmitry broke the old API for win32 with his DL_UNLOAD exposure
(which is where this redesign began). So a lot of modules simply won't work
there without a little massaging at present - not a huge problem within PHP
itself, but a niggling one for anyone with a non-public extension who
thought they could just upgrade to PHP 5.1.3 and up.

(A _very_ little massaging, but it's still a broken API.)

I think we need to fix that API breakage in 5_2 regardless of what happens
with this patch...

- Steph

Andi Gutmans

20 years ago

I read the whole thread.
I agree with Sara. While I understand where Steph is coming from, I
still think this is a good change. It would allow us to use this
method in the current tree right away, and for PECL authors who want
to stay compatible with older versions they can use the old & ugly
way of doing it until they feel comfortable in changing over.

I also second Sara on the naming changes of the macros. They ones
suggested by Dmitry didn't appeal to me either :)

Andi

At 05:15 AM 6/8/2006, Dmitry Stogov wrote:

Steph

20 years ago

Andi, hi,

>I read the whole thread.
> I agree with Sara. While I understand where Steph is coming from, I still
> think this is a good change. It would allow us to use this method in the
> current tree right away, and for PECL authors who want to stay compatible
> with older versions they can use the old & ugly way of doing it until they
> feel comfortable in changing over.

This redesign was presented as a fix for a win32 module API breakage in PHP
5.1.3 that causes shared modules to crash. Adopting the proposed changes now
won't help in any way for existing extensions, since everyone's saying 'oh
it'll be nice to use it in the future'. It just means we're stuck with the
status quo, and of course the odd confused extension author turning up with
a strange shutdown crash under win32 that didn't used to happen in PHP
5.1.2.

It'd make more sense to apply the patch partially in 5_2 and change the
module API number (to address that issue and give extension authors some
hope of understanding what's going on), and apply the whole thing in HEAD
and bump the API number again. I really think applying it in 5_2 is going to
leave extension writers with a headache.

- Steph

Rasmus Lerdorf

20 years ago

Steph Fox wrote:

> Andi, hi,
>
>
>> I read the whole thread.
>> I agree with Sara. While I understand where Steph is coming from, I
>> still think this is a good change. It would allow us to use this
>> method in the current tree right away, and for PECL authors who want
>> to stay compatible with older versions they can use the old & ugly way
>> of doing it until they feel comfortable in changing over.
>
> This redesign was presented as a fix for a win32 module API breakage in
> PHP 5.1.3 that causes shared modules to crash. Adopting the proposed
> changes now won't help in any way for existing extensions, since
> everyone's saying 'oh it'll be nice to use it in the future'. It just
> means we're stuck with the status quo, and of course the odd confused
> extension author turning up with a strange shutdown crash under win32
> that didn't used to happen in PHP 5.1.2.
>
> It'd make more sense to apply the patch partially in 5_2 and change the
> module API number (to address that issue and give extension authors some
> hope of understanding what's going on), and apply the whole thing in
> HEAD and bump the API number again. I really think applying it in 5_2 is
> going to leave extension writers with a headache.

As soon as you change the module api number you have a headache.  At
Yahoo! we would need to compile some 200 or so custom extensions.  If we
are forced to do that, a little tweak in the code itself is minor.

-Rasmus

Steph

20 years ago

Hi Rasmus,

>> It'd make more sense to apply the patch partially in 5_2 and change the
>> module API number (to address that issue and give extension authors some
>> hope of understanding what's going on), and apply the whole thing in HEAD
>> and bump the API number again. I really think applying it in 5_2 is going
>> to leave extension writers with a headache.
>
> As soon as you change the module api number you have a headache.  At
> Yahoo! we would need to compile some 200 or so custom extensions.  If we
> are forced to do that, a little tweak in the code itself is minor.

We aren't _forced_ to do that, I wrote a small patch over a week ago that
fixes it internally and doesn't even touch the module_entry struct. But the
more robust way would be to do as Dmitry's suggesting with the
ZEND_MG(whatever) part of his patch.

People with custom extensions at least are only working with one version of
PHP at a time, I'd hope :)

Pierre Joye

20 years ago

On 6/8/06, Steph Fox <steph@zend.com> wrote:

> People with custom extensions at least are only working with one version of
> PHP at a time, I'd hope :)

Not really. Especially not when you provide it as a product.

--Pierre

Derick Rethans

20 years ago

On Thu, 8 Jun 2006, Steph Fox wrote:

> People with custom extensions at least are only working with one version of
> PHP at a time, I'd hope :)

Are you kidding?  There are people using Xdebug for PHP 4.3.x, 4.4.x,
5.0.x, 5.1.x and 5.2.x... that's already enough headaches to maintain
with ifdefs all over the place.

regards,
Derick

Andi Gutmans

20 years ago

It's not going to be a headache because they can still use it the way
it should be done today.
For the 5.1.x tree you can do the fixes immediately with the way
Dmitry has previously proposed....

At 11:18 AM 6/8/2006, Steph Fox wrote:

Steph

20 years ago

> It's not going to be a headache because they can still use it the way it
> should be done today.

... for the last two months or so...  that's not a 'should', that's a
workaround.

> For the 5.1.x tree you can do the fixes immediately with the way Dmitry
> has previously proposed....

Huh? Did I miss something?

- Steph

Andi Gutmans

20 years ago

At 11:33 AM 6/8/2006, Steph Fox wrote:

>>It's not going to be a headache because they can still use it the
>>way it should be done today.
>
>... for the last two months or so...  that's not a 'should', that's
>a workaround.
>
>>For the 5.1.x tree you can do the fixes immediately with the way
>>Dmitry has previously proposed....
>
>Huh? Did I miss something?

I mean the infamous #ifdef in MSHUTDOWN :)

Steph, let me think about it more and review your patch again for 5.1.

Andi

Ilia A.

20 years ago

The patch is good, but I think this change maybe a bit too extreme
for PHP 5.2


On 8-Jun-06, at 8:15 AM, Dmitry Stogov wrote:

> Hi,
>
> The attached patch (for PHP_5_2) implements automatic management of
> module
> globals.
> The problem that module globals must be unregistered before extension
> unloading, because "globls_dtor" function is unloaded together with
> extension and cannot be called.
>
> To solve this problem extension writers now use the following pattern:
>
>  PHP_MSHUTDOWN_FUNCTION(mod_name)
>  {
> -#ifdef ZTS
> -	ts_free_id(mod_name_globals_id);
> -#else
> -	mod_name_globals_dtor(&mod_name_globals TSRMLS_CC);
> -#endif
>
> With my patch, extension writers should just extend module
> descriptor with
> globals descriptor and ctor/dtor callbacks.
>
>  	PHP_RSHUTDOWN(mod_name),
>  	PHP_MINFO(mod_name),
>  	NO_VERSION_YET,
> -	STANDARD_MODULE_PROPERTIES
> +	NULL,
> +	ZEND_MG(mod_name),
> +	ZEND_MGCTOR(mod_name),
> +	ZEND_MGCTOR(mod_name),
> +	STANDARD_MODULE_PROPERTIES_EX2
>  };
>
> Old extensions are source compatible and may work without
> modification.
> The patch modifies only several extensions, but will modify others
> too.
>
> I like commit the patch into HEAD and PHP_5_2.
> Any objections, additional ideas?
>
> Thanks. Dmitry.
> <zts_globals-2.diff.txt>

Ilia Alshanetsky

Pierre Joye

20 years ago

On 6/8/06, Ilia Alshanetsky <ilia@prohost.org> wrote:

> The patch is good, but I think this change maybe a bit too extreme
> for PHP 5.2

I agree. As far as I remember, we even discussed a similar solution
some years ago and decided to delay it until someone comes with a
better one, which indeed did not happen ;)

I do not see a problem to commit in HEAD and test it, no? We can
always add it later to 5.x. If 5.x still exists :-D

--Pierre

Pierre Joye

20 years ago

On Thu, 8 Jun 2006 23:34:10 +0200
pierre.php@gmail.com (Pierre) wrote:

> On 6/8/06, Ilia Alshanetsky <ilia@prohost.org> wrote:
> > The patch is good, but I think this change maybe a bit too extreme
> > for PHP 5.2
>
> I agree. As far as I remember, we even discussed a similar solution
> some years ago and decided to delay it until someone comes with a
> better one, which indeed did not happen ;)
>
> I do not see a problem to commit in HEAD and test it, no? We can
> always add it later to 5.x. If 5.x still exists :-D

I changed my mind, after having read again the patch (and roughly
tested), it would be good to have it in 5.2, the early the better :)

--  Pierre

Dmitry Stogov

20 years ago

Hi,

I will rework patch with new names (GINIT/GSHUTDOWN as Sara suggested) and
commit it into HEAD today or at Thursday.
According to PHP_5_2 we probably will need voting.

Thanks. Dmitry.

Derick Rethans

20 years ago

On Fri, 9 Jun 2006, Dmitry Stogov wrote:

> I will rework patch with new names (GINIT/GSHUTDOWN as Sara suggested) and
> commit it into HEAD today or at Thursday.
> According to PHP_5_2 we probably will need voting.

I'm -1 on integrating it into 5.2 then.

regards,
Derick

--
Derick Rethans
http://derickrethans.nl | http://ez.no | http://xdebug.org

Andi Gutmans

20 years ago

Why so? It doesn't affect any compatibility.

At 02:31 PM 6/8/2006, Ilia Alshanetsky wrote:

Steph

20 years ago

Andi,

> Why so? It doesn't affect any compatibility.

Yes it does, and more particularly it prevents any fix for the win32/ZTS
issue until people upgrade to the new module API. If everyone has to add an
extra #ifdef into MSHUTDOWN to get around the new situation, but then has to
take it away again 2 versions later... bleh... it's all wrong :)

Sara said earlier on IRC that it would be good to make the win32/ZTS thing a
separate issue.  I think so too.

Attached are patches for fixing the win32/ZTS thing only when needed - this
will _only_ work if you apply Dmitry's patch to 5_2 as well. The idea being
that this way everybody's covered.

It's similar for 5_1, except that it doesn't need the check for Dmitry's
changes.

I don't think we'll need my input for HEAD. As someone said earlier, people
will make changes for PHP 6.0 that they might not be ready or willing to
make for PHP 5.*. A good suggestion Sara came out with for PECL (this was
also on IRC) was to have some version check and offer up different #ext.c
files accordingly. At present we don't have that option, but we will if the
module API number is bumped following Dmitry's changes.

I still don't like it - but I'm quite good at smelling the way the goldfish
are swimming :-\

- Steph

Steph

20 years ago

I've done it again - this was an invisible patch :)

OK, relabelling the email. Nothing new to see here, move along...


----- Original Message -----
From: "Steph Fox" <steph@zend.com>
To: "Ilia Alshanetsky" <ilia@prohost.org>; "Dmitry Stogov"
<dmitry@zend.com>; "Andi Gutmans" <andi@zend.com>
Cc: <internals@lists.php.net>
Sent: Friday, June 09, 2006 5:22 AM
Subject: Re: [PHP-DEV] Re: [PATCH] Automatic module globals management

> Andi,
>
>> Why so? It doesn't affect any compatibility.
>
> Yes it does, and more particularly it prevents any fix for the win32/ZTS
> issue until people upgrade to the new module API. If everyone has to add
> an
> extra #ifdef into MSHUTDOWN to get around the new situation, but then has
> to
> take it away again 2 versions later... bleh... it's all wrong :)
>
> Sara said earlier on IRC that it would be good to make the win32/ZTS thing
> a
> separate issue.  I think so too.
>
> Attached are patches for fixing the win32/ZTS thing only when needed -
> this
> will _only_ work if you apply Dmitry's patch to 5_2 as well. The idea
> being
> that this way everybody's covered.
>
> It's similar for 5_1, except that it doesn't need the check for Dmitry's
> changes.
>
> I don't think we'll need my input for HEAD. As someone said earlier,
> people
> will make changes for PHP 6.0 that they might not be ready or willing to
> make for PHP 5.*. A good suggestion Sara came out with for PECL (this was
> also on IRC) was to have some version check and offer up different #ext.c
> files accordingly. At present we don't have that option, but we will if
> the
> module API number is bumped following Dmitry's changes.
>
> I still don't like it - but I'm quite good at smelling the way the
> goldfish
> are swimming :-\
>
> - Steph
>
>>
>> At 02:31 PM 6/8/2006, Ilia Alshanetsky wrote:
>>>The patch is good, but I think this change maybe a bit too extreme
>>>for PHP 5.2
>>>
>>>
>>>On 8-Jun-06, at 8:15 AM, Dmitry Stogov wrote:
>>>
>>>>Hi,
>>>>
>>>>The attached patch (for PHP_5_2) implements automatic management of
>>>>module
>>>>globals.
>>>>The problem that module globals must be unregistered before extension
>>>>unloading, because "globls_dtor" function is unloaded together with
>>>>extension and cannot be called.
>>>>
>>>>To solve this problem extension writers now use the following pattern:
>>>>
>>>>  PHP_MSHUTDOWN_FUNCTION(mod_name)
>>>>  {
>>>>-#ifdef ZTS
>>>>-       ts_free_id(mod_name_globals_id);
>>>>-#else
>>>>-       mod_name_globals_dtor(&mod_name_globals TSRMLS_CC);
>>>>-#endif
>>>>
>>>>With my patch, extension writers should just extend module
>>>>descriptor with
>>>>globals descriptor and ctor/dtor callbacks.
>>>>
>>>>         PHP_RSHUTDOWN(mod_name),
>>>>         PHP_MINFO(mod_name),
>>>>         NO_VERSION_YET,
>>>>-       STANDARD_MODULE_PROPERTIES
>>>>+       NULL,
>>>>+       ZEND_MG(mod_name),
>>>>+       ZEND_MGCTOR(mod_name),
>>>>+       ZEND_MGCTOR(mod_name),
>>>>+       STANDARD_MODULE_PROPERTIES_EX2
>>>>  };
>>>>
>>>>Old extensions are source compatible and may work without
>>>>modification.
>>>>The patch modifies only several extensions, but will modify others
>>>>too.
>>>>
>>>>I like commit the patch into HEAD and PHP_5_2.
>>>>Any objections, additional ideas?
>>>>
>>>>Thanks. Dmitry.
>>>><zts_globals-2.diff.txt>
>>>
>>>Ilia Alshanetsky
>>>
>>>
>>>
>>
>> --
>> PHP Internals - PHP Runtime Development Mailing List
>> To unsubscribe, visit: http://www.php.net/unsub.php
>>
>>
>> __________ NOD32 1.1380 (20060125) Information __________
>>
>> This message was checked by NOD32 antivirus system.
>> http://www.eset.com
>>
>>
>
>
> __________ NOD32 1.1380 (20060125) Information __________
>
> This message was checked by NOD32 antivirus system.
> http://www.eset.com
>
>

--------------------------------------------------------------------------------

Dmitry Stogov

20 years ago

Steph,

You patch is still need:

MSHUTDOWM(mod_name)
{
#ifndef ZTS
module_globals_dtor();
#endif

This is logical inconsistency.

My patch manages globals for both ZTS and non-ZTS builds and doesn't require
any tricks from extension writers.
The should just declare callbacks for module globals, and never call them
directly.

Thanks. Dmitry.

Steph

20 years ago

Dmitry, hi,

Sure - the difference is that that call's ALWAYS been needed in MSHUTDOWN
for non-ZTS dtor calls, whereas the thing of needing a ZTS call in MSHUTDOWN
is brand-new. The one's a 'feature', the other I think it's reasonable to
call a bug given that it's always worked until very recently (thanks to the
other bug that we didn't know about with the undefined HAVE_LIBDL).

I did work for a while on pushing the non-ZTS dtor pointers into a hash, but
I'm not good with function pointers so that attempt came to grief.... and as
I mentioned earlier, I don't know if it would've been too slow to do it that
way anyway.

What do you (and anyone?) think about the possibility of putting in my ZTS
patch alongside yours in 5_2, and probably something similar in isolation in
5_1 to fix the issue there, but leaving it out of HEAD? I'm hoping it'll
separate the issues of API redesign vs bugfix...

- Steph


----- Original Message -----
From: "Dmitry Stogov" <dmitry@zend.com>
To: "'Steph Fox'" <steph@zend.com>; "'Ilia Alshanetsky'" <ilia@prohost.org>;
"'Andi Gutmans'" <andi@zend.com>
Cc: <internals@lists.php.net>
Sent: Friday, June 09, 2006 9:35 AM
Subject: RE: [PHP-DEV] Re: [PATCH] Automatic module globals management


Steph,

You patch is still need:

MSHUTDOWM(mod_name)
{
#ifndef ZTS
module_globals_dtor();
#endif

This is logical inconsistency.

My patch manages globals for both ZTS and non-ZTS builds and doesn't require
any tricks from extension writers.
The should just declare callbacks for module globals, and never call them
directly.

Thanks. Dmitry.

> -----Original Message-----
> From: Steph Fox [mailto:steph@zend.com]
> Sent: Friday, June 09, 2006 7:23 AM
> To: Ilia Alshanetsky; Dmitry Stogov; Andi Gutmans
> Cc: internals@lists.php.net
> Subject: Re: [PHP-DEV] Re: [PATCH] Automatic module globals management
>
>
> Andi,
>
> > Why so? It doesn't affect any compatibility.
>
> Yes it does, and more particularly it prevents any fix for
> the win32/ZTS
> issue until people upgrade to the new module API. If everyone
> has to add an
> extra #ifdef into MSHUTDOWN to get around the new situation,
> but then has to
> take it away again 2 versions later... bleh... it's all wrong :)
>
> Sara said earlier on IRC that it would be good to make the
> win32/ZTS thing a
> separate issue.  I think so too.
>
> Attached are patches for fixing the win32/ZTS thing only when
> needed - this
> will _only_ work if you apply Dmitry's patch to 5_2 as well.
> The idea being
> that this way everybody's covered.
>
> It's similar for 5_1, except that it doesn't need the check
> for Dmitry's
> changes.
>
> I don't think we'll need my input for HEAD. As someone said
> earlier, people
> will make changes for PHP 6.0 that they might not be ready or
> willing to
> make for PHP 5.*. A good suggestion Sara came out with for
> PECL (this was
> also on IRC) was to have some version check and offer up
> different #ext.c
> files accordingly. At present we don't have that option, but
> we will if the
> module API number is bumped following Dmitry's changes.
>
> I still don't like it - but I'm quite good at smelling the
> way the goldfish
> are swimming :-\
>
> - Steph
>
> >
> > At 02:31 PM 6/8/2006, Ilia Alshanetsky wrote:
> >>The patch is good, but I think this change maybe a bit too
> extreme for
> >>PHP 5.2
> >>
> >>
> >>On 8-Jun-06, at 8:15 AM, Dmitry Stogov wrote:
> >>
> >>>Hi,
> >>>
> >>>The attached patch (for PHP_5_2) implements automatic
> management of
> >>>module globals.
> >>>The problem that module globals must be unregistered
> before extension
> >>>unloading, because "globls_dtor" function is unloaded together with
> >>>extension and cannot be called.
> >>>
> >>>To solve this problem extension writers now use the following
> >>>pattern:
> >>>
> >>>  PHP_MSHUTDOWN_FUNCTION(mod_name)
> >>>  {
> >>>-#ifdef ZTS
> >>>-       ts_free_id(mod_name_globals_id);
> >>>-#else
> >>>-       mod_name_globals_dtor(&mod_name_globals TSRMLS_CC);
> >>>-#endif
> >>>
> >>>With my patch, extension writers should just extend module
> descriptor
> >>>with globals descriptor and ctor/dtor callbacks.
> >>>
> >>>         PHP_RSHUTDOWN(mod_name),
> >>>         PHP_MINFO(mod_name),
> >>>         NO_VERSION_YET,
> >>>-       STANDARD_MODULE_PROPERTIES
> >>>+       NULL,
> >>>+       ZEND_MG(mod_name),
> >>>+       ZEND_MGCTOR(mod_name),
> >>>+       ZEND_MGCTOR(mod_name),
> >>>+       STANDARD_MODULE_PROPERTIES_EX2
> >>>  };
> >>>
> >>>Old extensions are source compatible and may work without
> >>>modification. The patch modifies only several extensions, but will
> >>>modify others too.
> >>>
> >>>I like commit the patch into HEAD and PHP_5_2.
> >>>Any objections, additional ideas?
> >>>
> >>>Thanks. Dmitry.
> >>><zts_globals-2.diff.txt>
> >>
> >>Ilia Alshanetsky
> >>
> >>
> >>
> >
> > --
> > PHP Internals - PHP Runtime Development Mailing List
> > To unsubscribe, visit: http://www.php.net/unsub.php
> >
> >
> > __________ NOD32 1.1380 (20060125) Information __________
> >
> > This message was checked by NOD32 antivirus system.
> > http://www.eset.com
> >
> >
>

__________ NOD32 1.1380 (20060125) Information __________

This message was checked by NOD32 antivirus system.
http://www.eset.com

Steph

20 years ago

Hey Dmitry we both need to learn to read to the end before we reply :)

> Attached are patches for fixing the win32/ZTS thing only when
> needed - this
> will _only_ work if you apply Dmitry's patch to 5_2 as well.
> The idea being that this way everybody's covered.

- Steph

----- Original Message -----
From: "Dmitry Stogov" <dmitry@zend.com>
To: "'Steph Fox'" <steph@zend.com>; "'Ilia Alshanetsky'" <ilia@prohost.org>;
"'Andi Gutmans'" <andi@zend.com>
Cc: <internals@lists.php.net>
Sent: Friday, June 09, 2006 9:35 AM
Subject: RE: [PHP-DEV] Re: [PATCH] Automatic module globals management

Steph,

You patch is still need:

MSHUTDOWM(mod_name)
{
#ifndef ZTS
module_globals_dtor();
#endif

This is logical inconsistency.

My patch manages globals for both ZTS and non-ZTS builds and doesn't require
any tricks from extension writers.
The should just declare callbacks for module globals, and never call them
directly.

Thanks. Dmitry.

> -----Original Message-----
> From: Steph Fox [mailto:steph@zend.com]
> Sent: Friday, June 09, 2006 7:23 AM
> To: Ilia Alshanetsky; Dmitry Stogov; Andi Gutmans
> Cc: internals@lists.php.net
> Subject: Re: [PHP-DEV] Re: [PATCH] Automatic module globals management
>
>
> Andi,
>
> > Why so? It doesn't affect any compatibility.
>
> Yes it does, and more particularly it prevents any fix for
> the win32/ZTS
> issue until people upgrade to the new module API. If everyone
> has to add an
> extra #ifdef into MSHUTDOWN to get around the new situation,
> but then has to
> take it away again 2 versions later... bleh... it's all wrong :)
>
> Sara said earlier on IRC that it would be good to make the
> win32/ZTS thing a
> separate issue.  I think so too.
>
> Attached are patches for fixing the win32/ZTS thing only when
> needed - this
> will _only_ work if you apply Dmitry's patch to 5_2 as well.
> The idea being
> that this way everybody's covered.
>
> It's similar for 5_1, except that it doesn't need the check
> for Dmitry's
> changes.
>
> I don't think we'll need my input for HEAD. As someone said
> earlier, people
> will make changes for PHP 6.0 that they might not be ready or
> willing to
> make for PHP 5.*. A good suggestion Sara came out with for
> PECL (this was
> also on IRC) was to have some version check and offer up
> different #ext.c
> files accordingly. At present we don't have that option, but
> we will if the
> module API number is bumped following Dmitry's changes.
>
> I still don't like it - but I'm quite good at smelling the
> way the goldfish
> are swimming :-\
>
> - Steph
>
> >
> > At 02:31 PM 6/8/2006, Ilia Alshanetsky wrote:
> >>The patch is good, but I think this change maybe a bit too
> extreme for
> >>PHP 5.2
> >>
> >>
> >>On 8-Jun-06, at 8:15 AM, Dmitry Stogov wrote:
> >>
> >>>Hi,
> >>>
> >>>The attached patch (for PHP_5_2) implements automatic
> management of
> >>>module globals.
> >>>The problem that module globals must be unregistered
> before extension
> >>>unloading, because "globls_dtor" function is unloaded together with
> >>>extension and cannot be called.
> >>>
> >>>To solve this problem extension writers now use the following
> >>>pattern:
> >>>
> >>>  PHP_MSHUTDOWN_FUNCTION(mod_name)
> >>>  {
> >>>-#ifdef ZTS
> >>>-       ts_free_id(mod_name_globals_id);
> >>>-#else
> >>>-       mod_name_globals_dtor(&mod_name_globals TSRMLS_CC);
> >>>-#endif
> >>>
> >>>With my patch, extension writers should just extend module
> descriptor
> >>>with globals descriptor and ctor/dtor callbacks.
> >>>
> >>>         PHP_RSHUTDOWN(mod_name),
> >>>         PHP_MINFO(mod_name),
> >>>         NO_VERSION_YET,
> >>>-       STANDARD_MODULE_PROPERTIES
> >>>+       NULL,
> >>>+       ZEND_MG(mod_name),
> >>>+       ZEND_MGCTOR(mod_name),
> >>>+       ZEND_MGCTOR(mod_name),
> >>>+       STANDARD_MODULE_PROPERTIES_EX2
> >>>  };
> >>>
> >>>Old extensions are source compatible and may work without
> >>>modification. The patch modifies only several extensions, but will
> >>>modify others too.
> >>>
> >>>I like commit the patch into HEAD and PHP_5_2.
> >>>Any objections, additional ideas?
> >>>
> >>>Thanks. Dmitry.
> >>><zts_globals-2.diff.txt>
> >>
> >>Ilia Alshanetsky
> >>
> >>
> >>
> >
> > --
> > PHP Internals - PHP Runtime Development Mailing List
> > To unsubscribe, visit: http://www.php.net/unsub.php
> >
> >
> > __________ NOD32 1.1380 (20060125) Information __________
> >
> > This message was checked by NOD32 antivirus system.
> > http://www.eset.com
> >
> >
>

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php


__________ NOD32 1.1380 (20060125) Information __________

This message was checked by NOD32 antivirus system.
http://www.eset.com

Stanislav Malyshev

20 years ago

SF>>Attached are patches for fixing the win32/ZTS thing only when needed -
SF>>this will _only_ work if you apply Dmitry's patch to 5_2 as well. The
SF>>idea being that this way everybody's covered.

I'm not sure I understand what this line does:
ts_free_id(table_size - zend_dl_module_count());

Can you explain?

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/ +972-3-6139665 ext.115

Dmitry Stogov

20 years ago

I also think that Steph's patch is to magic. :)

Dmitry.

Steph

20 years ago

What's 'magic' about going 1, 2, 3, 4....?

----- Original Message -----
From: "Dmitry Stogov" <dmitry@zend.com>
To: "'Stanislav Malyshev'" <stas@zend.com>; "'Steph Fox'" <steph@zend.com>
Cc: <internals@lists.php.net>
Sent: Friday, June 09, 2006 11:42 AM
Subject: RE: [PHP-DEV] Re: [PATCH] Automatic module globals management

Dmitry Stogov

20 years ago

Who said that 1,2,3,... should work?

What will your patch do if some extension will register several tsrm ids?

What will it do if some extension will not register globals?

Thanks. Dmitry.

Steph

20 years ago

Hi :)

> Who said that 1,2,3,... should work?

Erm, it does...

> What will your patch do if some extension will register several tsrm ids?

If an extension has several tsrm ids and it's loaded as a shared module, it
will probably crash on shutdown unless the author adds the appropriate
ts_free_id() calls in MSHUTDOWN - depending on the number of shared
extensions loaded, the load order in the php.ini, on the nature of the
modules unloading before it, and on how many tsrm ids it declares. (That
crash might well appear random.) The best way to deal with an extension
declaring more than one thread resource would be to build it as static. But
it actually wouldn't be any worse off than currently, because it would
already need to have ts_free_id() calls coming out of its ears to work as
shared at all, this side of 5.1.3.

> What will it do if some extension will not register globals?

That's the situation already with gd... Remember only dl'd modules _request_
their resources freed in this way - the other 20-odd resource 'owners' in
the core either wait until tsrm_shutdown(), or make an independant call to
ts_free_id(), or else are freed slightly early because of something like gd
being in the DL_UNLOAD queue. It doesn't matter when that happens - the
unload order isn't changed, the same resource id can't be freed twice, and
all the other actions within the module_destructor are module-related rather
than TSRM-related.

Stanislav Malyshev

20 years ago

SF>>crash might well appear random.) The best way to deal with an extension
SF>>declaring more than one thread resource would be to build it as static. But

That's a good advice but not practical at all. :) Static extensions can
not be distributed as a product or a package, for example.

SF>>being in the DL_UNLOAD queue. It doesn't matter when that happens -
SF>>the unload order isn't changed, the same resource id can't be freed
SF>>twice, and all the other actions within the module_destructor are
SF>>module-related rather than TSRM-related.

What if resource dtor of previous module uses some global from
later-freed module (which is perfectly legal as long as the order is
correct) and you free wrong ID? Kaboom.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/ +972-3-6139665 ext.115

Steph

20 years ago

Hi Stas,

> SF>>Attached are patches for fixing the win32/ZTS thing only when needed -
> SF>>this will _only_ work if you apply Dmitry's patch to 5_2 as well. The
> SF>>idea being that this way everybody's covered.
>
> I'm not sure I understand what this line does:
> ts_free_id(table_size - zend_dl_module_count());
>
> Can you explain?

OK, grab a coffee...

zend_dl_module_count() counts the modules as they enter module_destructor().
(Remember that dl'd modules are of necessity loaded last of all, and that
shutdown is in direct reverse order of loading.)

The number of the final TSRM resource id that was loaded will be the size of
the table they're stored in, plus one (to keep life simple). So we pick up
that figure and call it 'table_size'. Then we subtract the number of modules
registered in the dl_module_count.

That's actually the only way to figure out the resource id for any given
module without storing it in the module itself (which this patch checks for,
in terms of Dmitry's patch - I'm not altogether clear on what his does at
this point but they'll need to avoid each other).

You can't store the id during startup because there are resource ids that
are _not_ modules further down in the list and there's no way to know how
many of those there are, so you'll get up to about 12 with internal modules
and then suddenly the next number you can pick up is 20-something and static
because all the resource id's were already allocated before the module
registry kicked in.

Since dynamically loaded modules are the last ones to load, they're the
first to unload. They're checked for a handle, and if they have one they're
dl'd and we need to free the resource before unloading the module. We _only_
care about dl'd modules at this point. Nothing else actually needs its
resource freeing before tsrm_shutdown() (which cleans up everything that
wasn't freed already and is totally independent of this bit of maths), and
nothing else is touched in that way.

Anything may call ts_free_id() in its own right from its own source, but it
will have a known ID in that case anyway (so not an issue) and I added a
check to prevent double calls there, mainly because several extensions have
it currently.

This id generation is not 100% foolproof, sure - for example ext/gd doesn't
have any dealings with TSRM at all, and there's no way to know about that
from the data the module carries, so when it comes through as a shared
module it actually frees the resources for the next module in the shutdown
list. But freeing a module's resources slightly early during shutdown
doesn't do it any harm anyway - and the big thing here's to guarantee the
resources are freed before any module's unloaded, which this does because it
starts from the top, and to make sure you don't attempt to free a
non-existent resource id, which this also does because it can't touch
anything very far down in the shutdown list.

- Steph

Stanislav Malyshev

20 years ago

SF>>The number of the final TSRM resource id that was loaded will be the
SF>>size of the table they're stored in, plus one (to keep life simple).
SF>>So we pick up that figure and call it 'table_size'. Then we subtract
SF>>the number of modules registered in the dl_module_count.

I'm not sure why is this conclusion. Do you suppose only modules allocate
IDs and each of them allocates exactly one? What about Zend extensions
that also allocate IDs but do not have modules? What about modules not
needing globals? And what about dl()ed modules? Unless I am missing some
cruicial bits, this assumption seems to me very dangerous one.

SF>>Since dynamically loaded modules are the last ones to load, they're the

Are you sure they are? You can load an extension on another extension
startup, for example. Or on module startup. Or on any other stage of
startup procedure, for that matter, starting with module startup. You seem
to be relying on very precise order of events for this to work, which is
not guaranteed to happen.

SF>>care about dl'd modules at this point. Nothing else actually needs its
SF>>resource freeing before tsrm_shutdown() (which cleans up everything that

Zend extensions do too.

SF>>list. But freeing a module's resources slightly early during shutdown
SF>>doesn't do it any harm anyway - and the big thing here's to guarantee the

I don't like it at all. Really. If the code does not do what it was
intended to do, it's no excuse to say "well, nothing really bad happened"
- maybe not now, but what will happen in a year when another dozen of
extensions arrives? If the code doesn't do exactly the clearly defined
things it is supposed to do - we are literally inviting trouble for
ourselves later on the way. I thin such code is way too dangerous.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/ +972-3-6139665 ext.115

Steph

20 years ago

> SF>>The number of the final TSRM resource id that was loaded will be the
> SF>>size of the table they're stored in, plus one (to keep life simple).
> SF>>So we pick up that figure and call it 'table_size'. Then we subtract
> SF>>the number of modules registered in the dl_module_count.
>
> I'm not sure why is this conclusion.

Because we need to work through one resource at a time from the top, and
because dl'd modules are the last things with a resource id to be loaded,
because they have to be.

Do you suppose only modules allocate

> IDs and each of them allocates exactly one?

See my note to Dmitry - anything that allocates more than one resource is
broken for shared loading anyway unless it frees it from its own MSHUTDOWN.

What about Zend extensions

> that also allocate IDs but do not have modules?

You got me there. OK, I didn't know that happens until just now. The rest is
fine - really. But ... do Zend extensions actually use TSRM at all? - I
looked at XDebug, it uses TSRM if it's loaded as a PHP module, but not in
zend_extension mode. Is there some reason for that? Then the
zend_extensions_startup_mechanism() inits the llist for extensions some way
before the module registry kicks off. Doesn't that kind of imply that
zend_extensions load before modules are registered? The zend_extension llist
is destroyed _after_ the module registry is, which kind of backs up that
theory. I should test it, but looking at what happens there - I don't see it
causing problems.

> What about modules not needing globals?

I already went through that twice today...

>And what about dl()ed modules? Unless I am missing some
> cruicial bits, this assumption seems to me very dangerous one.

You're missing some crucial bits. You're assuming that the id that is freed
is actually that module's id, for a start. It isn't, it's just the next id
in the list - but only the number of dl'd modules is ever counted.

> SF>>Since dynamically loaded modules are the last ones to load, they're
> the
>
> Are you sure they are? You can load an extension on another extension
> startup, for example. Or on module startup. Or on any other stage of
> startup procedure, for that matter, starting with module startup. You seem
> to be relying on very precise order of events for this to work, which is
> not guaranteed to happen.

The key there is 'starting with module startup'. For a start there are two
distinct branches of module startup - the internal module startup, which
only deals with built-in modules, and the module registry, which is where
everything is actually loaded. You do actually need a core loaded to load
those modules into - that particular aspect of the load order can't change.
It's all about dependency - and that's exactly why the unload order _has_ to
reflect it and reverse it.

> SF>>care about dl'd modules at this point. Nothing else actually needs its
> SF>>resource freeing before tsrm_shutdown() (which cleans up everything
> that
>
> Zend extensions do too.

Yes, but since they're loading earlier than modules there's no impact there.

> SF>>list. But freeing a module's resources slightly early during shutdown
> SF>>doesn't do it any harm anyway - and the big thing here's to guarantee
> the
>
> I don't like it at all. Really. If the code does not do what it was
> intended to do, it's no excuse to say "well, nothing really bad happened"
> - maybe not now, but what will happen in a year when another dozen of
> extensions arrives? If the code doesn't do exactly the clearly defined
> things it is supposed to do - we are literally inviting trouble for
> ourselves later on the way. I thin such code is way too dangerous.

You're misunderstanding me. The only thing that can happen is that the next
module in the list has its resource_id freed near the end of a loop rather
than at the start of the next loop. Because the count only considers dl'd
modules, at worst that means a couple of the builtin extensions might get
freed 'early' in that sense. Half of them already call ts_free_id() anyway -
the impact of doing that is absolutely not negative.

The only thing in the equation I don't 100% know about and haven't tested is
Zend extensions - I'm going on the code alone for that.

- Steph

Stanislav Malyshev

20 years ago

SF>>Because we need to work through one resource at a time from the top,
SF>>and because dl'd modules are the last things with a resource id to be
SF>>loaded, because they have to be.

As you see belowe, this is not correct.

SF>>You got me there. OK, I didn't know that happens until just now. The rest is
SF>>fine - really. But ... do Zend extensions actually use TSRM at all? - I

Sure they do. Why shouldn't they?

SF>>I already went through that twice today...

And not once the answer "well, they are going to die anyway, so who cares
if we free ID for wrong module?" was satisfactory. I think it's not an
acceptable behaviour for the engine.

SF>>You're missing some crucial bits. You're assuming that the id that is freed
SF>>is actually that module's id, for a start. It isn't, it's just the next id

That's wrong assumption, because it bases on assumption that each module
allocates exactly one ID and only modules allocate IDs. Both of these are
wrong.

SF>>Yes, but since they're loading earlier than modules there's no impact
SF>>there.

Who said all extensions would load before all modules? What if extension
loads a module?

SF>>You're misunderstanding me. The only thing that can happen is that the
SF>>next module in the list has its resource_id freed near the end of a
SF>>loop rather than at the start of the next loop. Because the count only
SF>>considers dl'd modules, at worst that means a couple of the builtin
SF>>extensions might get freed 'early' in that sense. Half of them already
SF>>call ts_free_id() anyway - the impact of doing that is absolutely not
SF>>negative.

The code I see in the patch as I see it may very well call ts_free_id for
a module TSRM resource before module->module_shutdown_func is called, if
the formula (table_size - zend_dl_module_count()) gets wrong id. As there
never was a condition that shutdown_func may not use globals before they
are destroyed, the result would be module attempting to access already
free resource. Please explain why this situation is not possible - I see
the code is in module_destructor as follows:

if (module->module_started && module->module_shutdown_func) {
module->module_shutdown_func(module->type, module->module_number TSRMLS_CC);
}
module->module_started=0;
/.../
if (module->handle) {
if (!module->globals_id_ptr) {
/.../
ts_free_id(table_size - zend_dl_module_count());
}
DL_UNLOAD(module->handle);
}

So there's actually shutdown call between ts_free_id calls.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/ +972-3-6139665 ext.115

Steph

20 years ago

> SF>>Because we need to work through one resource at a time from the top,
> SF>>and because dl'd modules are the last things with a resource id to be
> SF>>loaded, because they have to be.
>
> As you see belowe, this is not correct.
>
> SF>>You got me there. OK, I didn't know that happens until just now. The
> rest is
> SF>>fine - really. But ... do Zend extensions actually use TSRM at all? -
> I
>
> Sure they do. Why shouldn't they?

Doesn't matter, they still load first.

>
> SF>>I already went through that twice today...
>
> And not once the answer "well, they are going to die anyway, so who cares
> if we free ID for wrong module?" was satisfactory. I think it's not an
> acceptable behaviour for the engine.

Have you even tested it?

> SF>>You're missing some crucial bits. You're assuming that the id that is
> freed
> SF>>is actually that module's id, for a start. It isn't, it's just the
> next id
>
> That's wrong assumption, because it bases on assumption that each module
> allocates exactly one ID and only modules allocate IDs. Both of these are
> wrong.

No. I am only 'talking' to DL'd modules here. When the DL'd module list is
cleared this doesn't get called any more. And as for assuming that each
module allocates exactly one ID - for the third time, any module doing this
won't work anyway without explicitly freeing those threads during MSHUTDOWN.
It doesn't make any difference in that situation.

> SF>>Yes, but since they're loading earlier than modules there's no impact
> SF>>there.
>
> Who said all extensions would load before all modules? What if extension
> loads a module?

It'd be difficult for an unloaded extension to load a module IMHO :)

> SF>>You're misunderstanding me. The only thing that can happen is that the
> SF>>next module in the list has its resource_id freed near the end of a
> SF>>loop rather than at the start of the next loop. Because the count only
> SF>>considers dl'd modules, at worst that means a couple of the builtin
> SF>>extensions might get freed 'early' in that sense. Half of them already
> SF>>call ts_free_id() anyway - the impact of doing that is absolutely not
> SF>>negative.
>
> The code I see in the patch as I see it may very well call ts_free_id for
> a module TSRM resource before module->module_shutdown_func is called, if
> the formula (table_size - zend_dl_module_count()) gets wrong id. As there
> never was a condition that shutdown_func may not use globals before they
> are destroyed, the result would be module attempting to access already
> free resource. Please explain why this situation is not possible - I see
> the code is in module_destructor as follows:
>
>    if (module->module_started && module->module_shutdown_func) {
>      module->module_shutdown_func(module->type, module->module_number
> TSRMLS_CC);
>    }
>    module->module_started=0;
> /.../
>        if (module->handle) {
> if (!module->globals_id_ptr) {
> /.../
>        ts_free_id(table_size - zend_dl_module_count());
>      }
>        DL_UNLOAD(module->handle);
> }
>
> So there's actually shutdown call between ts_free_id calls.

Erm, Stas, it's not because it's impossible - it's because that
module_shutdown_func happens to be the module's MSHUTDOWN. The contents of
MSHUTDOWN generally look something like:

#ifdef ZTS
ts_free_id(pcre_globals_id);
#else
php_pcre_shutdown_globals(&pcre_globals TSRMLS_CC);
#endif

UNREGISTER_INI_ENTRIES();

return SUCCESS;

I'm not changing a darn thing in the order of the calls there, and that's
*exactly* why I don't get your point.

- Steph

Ilia A.

20 years ago

You're right, Steph's comments threw me off a bit there, but after
reconsidering the patch in detail I think it should be fine for 5.2,
so go ahead and commit it.


On 8-Jun-06, at 7:44 PM, Andi Gutmans wrote:

> Why so? It doesn't affect any compatibility.
>
> At 02:31 PM 6/8/2006, Ilia Alshanetsky wrote:
>> The patch is good, but I think this change maybe a bit too extreme
>> for PHP 5.2
>>
>>
>> On 8-Jun-06, at 8:15 AM, Dmitry Stogov wrote:
>>
>>> Hi,
>>>
>>> The attached patch (for PHP_5_2) implements automatic management of
>>> module
>>> globals.
>>> The problem that module globals must be unregistered before
>>> extension
>>> unloading, because "globls_dtor" function is unloaded together with
>>> extension and cannot be called.
>>>
>>> To solve this problem extension writers now use the following
>>> pattern:
>>>
>>>  PHP_MSHUTDOWN_FUNCTION(mod_name)
>>>  {
>>> -#ifdef ZTS
>>> -       ts_free_id(mod_name_globals_id);
>>> -#else
>>> -       mod_name_globals_dtor(&mod_name_globals TSRMLS_CC);
>>> -#endif
>>>
>>> With my patch, extension writers should just extend module
>>> descriptor with
>>> globals descriptor and ctor/dtor callbacks.
>>>
>>>         PHP_RSHUTDOWN(mod_name),
>>>         PHP_MINFO(mod_name),
>>>         NO_VERSION_YET,
>>> -       STANDARD_MODULE_PROPERTIES
>>> +       NULL,
>>> +       ZEND_MG(mod_name),
>>> +       ZEND_MGCTOR(mod_name),
>>> +       ZEND_MGCTOR(mod_name),
>>> +       STANDARD_MODULE_PROPERTIES_EX2
>>>  };
>>>
>>> Old extensions are source compatible and may work without
>>> modification.
>>> The patch modifies only several extensions, but will modify others
>>> too.
>>>
>>> I like commit the patch into HEAD and PHP_5_2.
>>> Any objections, additional ideas?
>>>
>>> Thanks. Dmitry.
>>> <zts_globals-2.diff.txt>
>>
>> Ilia Alshanetsky
>>
>>
>>
>
>

Ilia Alshanetsky

Steph

20 years ago

My comments went something along the lines of 'well if it's going in can we
please apply a fix for the problem this was supposed to fix in the first
place, because it isn't an answer to that problem'. I'd still personally
rather it didn't, but I'm happy so long as we get an actual fix in there
rather than a temporary pain-in-the-ass bug we all have to work around for
the next two years.