[PATCH] Automatic module globals management

19 years ago by Steph Fox — view source

unread

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

--

__________ NOD32 1.1380 (20060125) Information __________

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

19 years ago by Sara Golemon — view source

unread

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

19 years ago by Steph Fox — view source

unread

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

19 years ago by Sara Golemon — view source

unread

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

19 years ago by Steph Fox — view source

unread

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

19 years ago by Sara Golemon — view source

unread

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

19 years ago by Steph Fox — view source

unread

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....

-Sara

19 years ago by Stanislav Malyshev — view source

unread

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

19 years ago by Steph Fox — view source

unread

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

19 years ago by Andi Gutmans — view source

unread

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:

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.

19 years ago by Steph Fox — view source

unread

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

19 years ago by Rasmus Lerdorf — view source

unread

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

19 years ago by Steph Fox — view source

unread

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 :)

-Rasmus

19 years ago by Pierre — view source

unread

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

19 years ago by Derick Rethans — view source

unread

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

19 years ago by Andi Gutmans — view source

unread

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:

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

19 years ago by Steph Fox — view source

unread

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

19 years ago by Andi Gutmans — view source

unread

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

19 years ago by Ilia Alshanetsky — view source

unread

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

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

19 years ago by Pierre — view source

unread

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

19 years ago by Pierre — view source

unread

On Thu, 8 Jun 2006 23:34:10 +0200
pierre.php@gmail.com (Pierre) 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

19 years ago by Andi Gutmans — view source

unread

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
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

19 years ago by Steph Fox — view source

unread

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
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
--

__________ NOD32 1.1380 (20060125) Information __________

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

19 years ago by Steph Fox — view source

unread

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
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
--

__________ 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

--

__________ NOD32 1.1380 (20060125) Information __________

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

19 years ago by Stanislav Malyshev — view source

unread

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

19 years ago by Steph Fox — view source

unread

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

19 years ago by Stanislav Malyshev — view source

unread

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

19 years ago by Steph Fox — view source

unread

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

19 years ago by Stanislav Malyshev — view source

unread

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

19 years ago by Steph Fox — view source

unread

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

19 years ago by Stanislav Malyshev — view source

unread

SF>>Erm, Stas, it's not because it's impossible - it's because that
SF>>module_shutdown_func happens to be the module's MSHUTDOWN. The contents of
SF>>MSHUTDOWN generally look something like:
SF>>
SF>>#ifdef ZTS
SF>>ts_free_id(pcre_globals_id);
SF>>#else
SF>>php_pcre_shutdown_globals(&pcre_globals TSRMLS_CC);
SF>>#endif
SF>>
SF>>UNREGISTER_INI_ENTRIES();
SF>>
SF>>return SUCCESS;

That's quite an assumption. What if it looks like:

if(MODULE_G(flag)) {
do_complicated_shutdown(MODULE_G(data));
}

before your code, and you have already freed the MODULE_G's id?

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

You are changing the order of ts_free_id's however - once you assumption
of "one alloc exactly per module" is wrong, you can have free_id before
dtor is called.

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

19 years ago by Ilia Alshanetsky — view source

unread

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.

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
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

19 years ago by Steph Fox — view source

unread

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.

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.
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
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