On Monday 14 May 2007 08:35, Philippe M. Chiasson wrote: > >> http://www.gossamer-threads.com/lists/modperl/dev/93415 > > > > This one looks pretty good, and I think it's a cleaner approach > > to the currently uncorrect one. > > I've just spent the evening looking at this more in details, and the > more I look, the more things look significantly broken. My latest > feeling was that your patch was not actually adressing the underlying > problem.
I was there myself, see http://www.gossamer-threads.com/lists/modperl/dev/92674#92674 > Here is where I am at so far. In many cases, once an interpreter has been > acquired and not yet released, nothing should really need to call > interp_select() to find an interpreter, as opposed to being handed that > interpreter directly. The point is you can go your way and prevent recursive calls. That'd mean redesign of the modperl_response_handler* functions at least. Or you can make modperl_interp_unselect reenterable and handle the recursion. That is what I did. I think refcounting is a quite good approach to that. I see interp_select() as the interface that hands you the *right* interpreter. It handles all the details of which interp and where it comes from. The thing that disturbs me a bit is the PUTBACK flag and its inspection all over the code. I think it would be cleaner to call select/unselect each time and let them decide what to do. And what disturbs me even more is the way select/unselect is called, sometimes though a macro, sometimes directly, sometimes conditionally ("if(c||r)" or so). It was very hard to figure out what was going on. The semantics of PerlCleanupHandler is a bit strange in case of scope==handler but I think it is quite good the way it is. It needs to be explained as a way to deallocate resources that are bound to a certain perl instance while a pool cleanup handler handles resources that are bound to the request/connection/process whatever the pool belongs to. > On the topic of interp->refcnt, check out xs/APR/Pool/APR_Pool.h, it's > the current customer of it, and uses it again in a strange/unobvious > way. > > Anyways, I'll be poking at this some more shortly and post more of my > findings. I really like the idea of refcounting. Initially I was looking for a way to allocate an interp from trans to fixup and release it just before response. This way most of the slow network IO is made without an interpreter. But it preserves the possibility to communicate via pnotes between the phases. With my patch and the way APR::Pool handles refcounts it can be done very simple: in trans: $r->pnotes->{my_pool}=$r->pool->new; this preserves the interp from being freed. Then in fixup: delete $r->pnotes->{my_pool}; And since the interp is stored in rcfg it is also the faster way compared with a scope==request. With scope==request||subrequest the interp is stored in the request pools userdata. That means a hash lookup instead of a simple structure access. It would be even faster if that initial subpool creation could be avoided. For that I think a Apache2::Interp or Modperl::Interp module would be good to access the refcnt. Would something like that be accepted? What would be the right name? I'd vote for something like rcfg->interp but not in rcfg but in a connection bound structure to always store the interp currently in use. Then decisions about the lifetime of that field are made completely in interp_(un)select(). This pair of functions is then called every time an interp is needed. For the scope==subreq case there must be something special (an apr_array) then I think. Another strangeness is that PerlInterpScope is of directory scope. What to do when there is a PerlTransHandler configured with scope==handler but in MapToStorage a PerlInterpScope directive says it is scope==subreq or so. This needs to be investigated I think. > Torsten, thanks for the ithreads3 test case, it does correctly exercice > the problems every time. (I'd like to include it, but it would need to > be somewhat cleaned up to follow our current coding conventions, apart > > >from that, pretty good). This weekend I made a patch to allow APR::ThreadRWLock by now without test. I'll see if it does what I expect it to do and then send it to the list. Torsten
pgpGSVELRvBXA.pgp
Description: PGP signature
