Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:24051
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Message-ID: <178401c68bf7$0818bde0$6602a8c0@foxbox>
Reply-To: "Steph Fox" <steph@zend.com>
To: "Stanislav Malyshev" <stas@zend.com>
Cc: <internals@lists.php.net>
References: <000001c68af5$50876930$6e02a8c0@thinkpad><7B45819D-6E8D-4B9B-A02B-BC00BE6B1930@prohost.org> <7.0.1.0.2.20060608164415.06b4ae50@zend.com> <133f01c68b73$fb1a1bd0$6602a8c0@foxbox> <Pine.LNX.4.64.0606091127001.17284@mail.zend.com> <145901c68bad$cdd4b470$6602a8c0@foxbox> <Pine.LNX.4.64.0606091732500.306@mail.zend.com> <165e01c68be9$afb4cb60$6602a8c0@foxbox> <Pine.LNX.4.64.0606092059200.17062@shire.zend.office>
Date: Fri, 9 Jun 2006 21:00:52 +0200
MIME-Version: 1.0
Content-Type: text/plain;
	format=flowed;
	charset="Windows-1252";
	reply-type=original
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Re: [PATCH] Automatic module globals management
From: steph@zend.com ("Steph Fox")

> 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