Re: Change FILE/... buckets to close file descriptor on destruction?

[Stefan Fritsch <sf...@sfritsch.de>]

On Tuesday 18 May 2010, Stefan Fritsch wrote:
> On Tuesday 18 May 2010, Joe Orton wrote:
> > On Tue, May 18, 2010 at 09:18:23AM +0200, Stefan Fritsch wrote:
> > > On Tue, 18 May 2010, Ruediger Pluem wrote:
> > > >So if you want to close this fd you IMHO would need to do some
> > > >refcounting and only close it if no other filebucket still
> > > >references it.
> > > 
> > > The filebuckets already do refcounting.
> > > apr_bucket_shared_destroy(f) in the patch above is only true if
> > > the refcount has reached zero.
> > 
> > They refcount the number of times the FILE bucket has been
> > split/copied, they don't refcount the number of times the
> > underlying apr_file_t object is used.
> > 
> > APR does not seem to be consistent about of whether "ownership"
> > of the object is passed on when you create a bucket wrapper for
> > that object type; there are no API guarantees or constraints
> > here. From a quick review, PIPE, MMAP, HEAP (kind of) do take
> > ownership, FILE and SOCKET do not.
> > 
> > Have you run the perl-framework test suite to see whether this
> > breaks anything?  This change does look like it'll break the APR
> > test suite but only because of the way I happened to write one of
> > the buckets tests.
> 
> It does not cause any breakage in the perl-framework. As you
> suspected, it does break apr-util's testbuckets test. I will look
> if I can run the subversion test suite, too.
> 
> If changing the current behaviour is considered too disruptive, one
> could also introduce a flag for the cleanup behaviour of FILE
> buckets, like there is a flag for mmap. IMHO doing the closing in
> the bucket cleanup is far preferable to doing it anywhere else in
> the core output filter.

From other mail:
> I found no breakage in subversion 1.6.11's test suite.


To get this item from the httpd 2.4 blockers list, we could vote:

[  ] change FILE/PIPE/SOCKET bucket in APR to close file descriptor
     when the last referencing bucket is destroyed
[  ] same as above, but only if a new flag is set (like flag for mmap)
[  ] don't change APR, work around problem in httpd

But I am not sure what introducing a new element in struct 
apr_bucket_file would mean for ABI compatibility. On the one hand, 
apr_bucket_file is public, on the other hand, there are functions like 
apr_bucket_file_create and apr_bucket_file_enable_mmap. Would adding 
an element at the end be ok for apr-util 1.4, or would that only be 
possible in 2.0?

Cheers,
Stefan

Re: Change FILE/... buckets to close file descriptor on destruction?

[Stefan Fritsch <sf...@sfritsch.de>]

On Monday 14 June 2010, Stefan Fritsch wrote:
> [  ] change FILE/PIPE/SOCKET bucket in APR to close file descriptor
>      when the last referencing bucket is destroyed
> [  ] same as above, but only if a new flag is set (like flag for
> mmap)
> [  ] don't change APR, work around problem in httpd

For the record: Because of the compatibility headaches this change 
would cause in APR, I have fixed the problem in httpd instead.

Re: Change FILE/... buckets to close file descriptor on destruction?

[Stefan Fritsch <sf...@sfritsch.de>]

On Monday 14 June 2010, William A. Rowe Jr. wrote:
> On 6/14/2010 4:20 PM, Stefan Fritsch wrote:
> > But I am not sure what introducing a new element in struct
> > apr_bucket_file would mean for ABI compatibility. On the one
> > hand, apr_bucket_file is public, on the other hand, there are
> > functions like apr_bucket_file_create and
> > apr_bucket_file_enable_mmap. Would adding an element at the end
> > be ok for apr-util 1.4, or would that only be possible in 2.0?
> 
> We could create a new apr_bucket_file_closing flavor, but no.  You
> can't alter the public structures until 2.0.  I really don't
> recall why these internals were exposed, but this does demonstrate
> that might have been a bad idea ;-)

Yes, that's unfortunate.
 
> As far as changing the structure itself, I wouldn't start creating
> two ints when it's simple to change can_mmap to a bit flag,
> likewise 'should be closed'.  But you are thinking of marking
> every bucket for this file with this flag?
> 
> I'm wondering if this isn't actually a more general problem that
> would be better solved with a broader hook-on-refcount==0 entity?

You think in the form of an apr_bucket_destroy_ex() function? Maybe 
that would be possible without changing the ABI (or maybe not, 
apr_bucket_type_t with all the function pointers is public, too). But 
this it no longer looking like a simple solution to me.

And I just remembered mod_file_cache, which caches open fds. It just 
calls apr_brigade_insert_file which in turn calls 
apr_bucket_file_create. This looks to me like it would be broken by 
APR simply changing the semantics of FILE buckets to close the fd.

Therefore I have changed my mind and now think that a solution inside 
httpd may be best. For example the core_output_filter could setaside 
the buckets not into the conn pool but into a temp pool that is 
cleared after the EOR bucket is seen. Does that sound reasonable?

Re: Change FILE/... buckets to close file descriptor on destruction?

["William A. Rowe Jr." <wr...@rowe-clan.net>]

On 6/14/2010 4:20 PM, Stefan Fritsch wrote:
> 
> But I am not sure what introducing a new element in struct 
> apr_bucket_file would mean for ABI compatibility. On the one hand, 
> apr_bucket_file is public, on the other hand, there are functions like 
> apr_bucket_file_create and apr_bucket_file_enable_mmap. Would adding 
> an element at the end be ok for apr-util 1.4, or would that only be 
> possible in 2.0?

We could create a new apr_bucket_file_closing flavor, but no.  You can't
alter the public structures until 2.0.  I really don't recall why these
internals were exposed, but this does demonstrate that might have been
a bad idea ;-)

As far as changing the structure itself, I wouldn't start creating two
ints when it's simple to change can_mmap to a bit flag, likewise 'should
be closed'.  But you are thinking of marking every bucket for this file
with this flag?

I'm wondering if this isn't actually a more general problem that would
be better solved with a broader hook-on-refcount==0 entity?