On Wed, Jan 27, 2016 at 10:02 AM, Plüm, Rüdiger, Vodafone Group <ruediger.pl...@vodafone.com> wrote: > >> -----Original Message----- >> From: Yann Ylavic [mailto:ylavic....@gmail.com] >> Sent: Mittwoch, 27. Januar 2016 09:15 >> To: httpd-dev >> Subject: Re: svn commit: r1726787 - >> /httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c >> >> I'm not sure the buckets can be destroyed while in flight in >> mod_proxy_wstunnel, unlike with mod_proxy_http where the backend >> connection/request may be cleared before all data are out. >> >> ISTM that this can't happen here because we flush out on every write > > In theory this should be the case, but I think having brigade and buckets > allocated from > the same bucket allocator is a "better safe than sorry" approach that catches > possible > edge case now or in the future (think of Grahams ideas to have suspending > filters). > The issue with these possible edge cases is that it will be hard to find the > root cause, > because we likely segfault somewhere else.
I agree that a safe approach is needed for buckets' lifetime from input to output filters. I'd prefer this to be handled at the source though (by the input filter and callees that fill in the brigade), possibly in ap_core_input_filter() and ap_filter_reinstate_brigade() only. These would ap_buckets_lifetime_transform() buckets from their aside brigade to the caller's one (which could be a simple move if the lifetime is already fine, ie. non proxy case). Maybe that would require some apr_bucket_copy_ex() (or setaside_ex()?) which would allow heap buffers (and file/mmap/..., via refcounting) moves from one bucket_alloc to another (given as _ex()'s arg). Possibly I'm minimizing this task :p Overall I think that could avoid unnecessary allocations/copies (which may still happen currently), putting this logic at the right place IMHO (the caller decides where the buffers are allocated according to its needs by creating the brigade, the input filters use bb->bucket_alloc (and bb->r) to create/move their buckets). > And in fact it should cost us not much here as the buckets are likely memory > buckets already > and nothing is really done with the content if no one sets it aside. Walking the brigades (and creating transient buckets) may be a noticeable overhead in the websocket case, I wish we could rely on the flush... > >> (btw that could be optimized to flush outside the loop). > > Could be. The question is, whether this is ok from the websockets > responsiveness point of view. > OTOH we do an non blocking read. So this should be fine. Yes, while successive non-blocking reads give us data I don't think we should "fragment" on the other side, let the core/socket buffers handle it. Regards, Yann.
