> Am 04.05.2016 um 13:49 schrieb Graham Leggett <minf...@sharp.fm>: > > On 04 May 2016, at 11:13 AM, Stefan Eissing <stefan.eiss...@greenbytes.de> > wrote: > >> The problem is not the apr_bucket_destroy(). The file bucket setaside, calls >> apr_file_setaside(), in core_output on a deferred pool, and then core_output >> clears that pool. This invalidates all still existing file buckets using >> that apr_file. > > This scenario should still work properly, it shouldn’t cause anything to > break. > > First off, file buckets need reference counting to make sure that only on the > last removal of the bucket the file is closed (it may already do this, I > haven’t looked, but then if it did do this properly it should work). > > Next, if a file bucket is setaside, but the reference count isn’t one (in > other words, other file buckets exist pointing at the same file descriptor in > other places), and the pool we’re setting aside into isn’t the same or a > child pool, we should dup the file descriptor during the setaside. > > The typical scenario for the deferred pool should be the first scenario above.
file_bucket_setaside() currently does not care about the refcount. setaside affects *all* shared file buckets, wherever they currently reside. So it moves the contained apr_file_t into the filter deferred pool, eventually clears that pool and the file is closed via cleanup of the file (not the bucket). While dup'ing the file descriptor would work, it seems overly costly in this case. What is the case? The output filter already makes the distinction wether filter->r is set or not. When filter->r is set, it uses filter->r->pool to setaside buckets it wants to keep around. This is safe since it knows that some time in the future, an EOR bucket will come along and cleanup - at the right time. HTTP/2 has a similar bucket lifetime case: only now, there are several requests ongoing and interleaved onto the one master connection. But the basic assumption still holds: there will be some kind of EOR bucket that manages the lifetimes of buckets before it correctly. But the output filter does not know this and even if, would not know which pool to setaside which bucket to. So. One way for a generic approach is a new META bucket: POOL_LIFE that carries a pool or NULL. It's contract is: all buckets that follow me have the lifetime of my pool (at least) and this holds until another POOL_LIFE bucket comes along. Pools announced in this way are promised to only disappear after some kind of EOR or FLUSH has been sent. Given that requests R1, R2, R3 with pools P1, P2, P3 are under way, a brigade passed to core output would look like this: [PL P1][R1 DATA][R1 DATA][PL P3][R3 DATA][PL P2][R2 EOR][PL P1][R1 DATA][PL NULL]... where the PL buckets would switch the setaside pool used by core_output. if no POOL_LIFE is seen or if the contained pool is NULL, the current defaults (r->pool / deferred) are used. Using pools instead of higher level, protocol specific entities (such as request_rec) should make that flexible enough for different use cases. Thoughts? -Stefan
