On 04 May 2016, at 3:22 PM, Stefan Eissing <stefan.eiss...@greenbytes.de> wrote:
> 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). That’s broken, we need to fix that so it works properly. > 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. That’s expected - during requests, we set aside into the request pool, where requests are one shot and you’re done. During connections however we cannot use the connection pool, because the connection pool lives potentially for a very long time. This is why the deferred pool exists. None of this matters though, buckets should work correctly in both cases. > 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. This breaks the contract of pools - every bucket has a pool, and now there would be a second mechanism that duplicates this first mechanism, and as soon as there is a mismatch we crash. Let’s just fix the original bug - make sure that file buckets behave correctly when setaside+refcount is used at the same time. Regards, Graham —
