On 01/27/2016 12:46 PM, Yann Ylavic wrote: > 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.
Just saw that the code for mod_proxy_connect is very similar to the one from mod_proxy_wstunnel and thus could impose the same issues. Maybe it should be fixed as well. > > 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. Why ap_core_input_filter? Not all filters are using ap_filter_reinstate_brigade() so far. Some just tuck away parts of the brigade in brigades stored in their filter context. As far as I can tell the only thing we say about that API wise is that the filter MUST setaside the buckets then. Nothing more. So IMHO we would need to make the API more tight here for the consumers if we have other expectations. > 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 Haven't looked at this so far. Hence I do not know how difficult it is to do that, but the current approach especially for heap buckets looks efficient to me. A read on the heap bucket is nearly for free as is a creation of a transient bucket with the buffer of the heap bucket. Only in case of a setaside we copy the memory and I see no way how to change the lifetime of the data in the buffer by other means. And this all happens with the current code :-) > 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). Not really getting the idea so far. Can you please elaborate? > >> 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... I agree that it is some overhead, but IMHO the improved safety / stability outweighs this overhead. But I agree that this is subject to personal opinion somehow. > >> >>> (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. I am fine with flushing at the end of the loop. If no one opposes I can do this. Regards Rüdiger
