On Wed, 2002-08-21 at 02:59, Justin Erenkrantz wrote: > On Tue, Aug 20, 2002 at 09:54:43PM +1000, Bojan Smojver wrote: > > After reading a bit more from the function core_input_filter, here is > > what I can conclude: > > > > - to get to request from the filter, one would use f->r in that function > > - since there is no r->bytes_read in current request_rec we could: > > - introduce one and break compatibility > > - store the bytes_read as in r->notes, which is easy but not right > > All that would require is a MMN bump. (We're just adding a field, > so that's what a MMN bump is for.)
Yeah, I ran into that one when compiling mod_jk and PHP. So, that's what it's for... ;-) Anyway, there is a note about 64-bit alignment of request_rec, so maybe it's a good chance for someone familiar with those systems to rearrange the record accordingly. > > - the actual number of bytes read is stored in local variable len: > > - the r->bytes_read would just be: > > > > f->r->bytes_read += len; > > > > after each read > > Indeed. > > > So, r->bytes_sent would then be completely different that in the current > > version of Apache 1.3/2.0, which simply reflects the length of the body? > > Would others be inclined to accept such a change? > > Totally. The current behavior is broken. We all pretty much agree > that it is wrong. Excellent. As long as nobody depends upon that broken behaviour... > > Everything here (core_output_filter) revolves around apr_brigade_write > > and local variable n, which looks like the number of bytes that are > > about to go down the pipe: > > > > - to establish the total, we do: > > > > f->r->bytes_sent += n; > > > > after each write > > > > All other increments of r->bytes_sent have to be removed. I somehow > > don't believe it's that simple... Or is it? > > I believe it is. > > Now, there is *one* caveat on bytes_sent - keepalive connection may > not actually be written in the lifetime of that request (if the > response is small enough, we'll hold on to it until the next response > is ready and write both at the same time). > > That could end up with bytes_sent being 0 since we didn't write the > response immediately. On the next response, that bytes_sent value > would be the value of *both* responses. Ick, but I'm not entirely > clear how to get around that just yet. -- justin Do we really need to get around it? It seems like the information about what really happened. The next request got more writing, so its fair to attribute that to it. Which then brings me to another point - maybe we shouldn't introduce just one new field into record_rec, but two fields. For instance bytes_pushed and bytes_pulled, which would record the above (i.e. the number of bytes actually read/written for the request, as per core_input_filter and core_output_filter). Those can then be used for logging purposes and statistics, together with the old bytes_sent. bytes_sent would then get the logic from log_bytes_sent_all (from the patch), to make sure it is accurate, however, done way before mod_log_config. Since this is not the input from the client, we can actually determine the correct length of the response, including status line and all headers. So, there could be requests with bytes_pushed zero, which would then correctly reflect the fact that there was no writing during that request. The final statistics would also be correct, as those responses would be written later on. The alternative is to correct bytes_sent as per above, which is not difficult, and calculate only bytes_read in core_input_filter. We'd save a field while retaining accuracy per request and not knowing what happened with keepalive connections (not a big deal in my view). I'm leaning more toward this solution, if that makes any difference. Bojan
