https://bz.apache.org/bugzilla/show_bug.cgi?id=56729
--- Comment #27 from Yann Ylavic <[email protected]> --- (In reply to Michael Kaufmann from comment #26) > > Probably does not work too (not tested) because it has the same problem as > the previous patch (see below) You possibly should test it... > > The worker MPM calls the hook "pre_read_request" when a request has been > processed, *before* the keep-alive state. The event MPM calls this hook when > the next request has been detected, *after* the keep-alive state. I think we need to agree on what the keepalive state is. >From mod_reqtimeout POV, this is when the time taken waiting for the next request should not be taken into account, which happens to be when called from read_request_line(). This report is about subsequent requests possibly timedout by the module because the keep-alive time becomes accounted after some speculative read (from check_pipeline) resets the state. After r1621453 and r1641376 (which BTW I assume you applied already), the new issue is that the log_transaction hook may happen either before or after the subsequent request is processed, depending on whether the previous request has already been destroyed or not (e.g. pipelined responses). In the latter case, the keepalive state may not be set (or too late, or twice, ..., that's a race). With the latest patch, read_request_line() is called just after the pre_read_request hook (which indeed happens after the previous request is processed, but mainly *before* the one mod_reqtimeout's state is for). The worker MPM calls read_request_line() directly to read the next request, so it will pass the keep-alive timeout time in reqtimeout_filter(), we must not account it, nor depend on the previous request to be destroyed. The event MPM handles the kept-alive connections by itself (polling the socket directly, without using the input filter stack), consequently when read_request_line() is then called, the current (as per the latest patch) request is indeed already available, yet we can't depend on the previous request to update the state. That's what the latest patch addresses, the state is initialized in the pre_read_request hook ([be]for[e] the new request) by reqtimeout_before_headers(), thus the racy log_transaction hook reqtimeout_after_body() is not needed anymore. The patch now also account for the request line read itself (after the wait), whereas the original code simply forwarded the result with no timeout check (subsequent requests only). Hopefully it would solve your issue since now I can't see how the state could be wrong (from moq_reqt POV...). > > Example: > > Detect incoming data > Process request 1 > Finish request 1 worker and event could have called ap_run_log_transaction() here, or not... > --> worker calls the pre_read_request hooks here > ... wait for next request (keep-alive state) ... worker::ap_read_request()::ap_run_pre_read_request()::reqtimeout_before_headers() worker::process_socket()::ap_process_connection():..:ap_read_request()::read_request_line()::reqtimeout_filter() Whereas: event::[listen]::poll() > Detect incoming data > --> event calls the pre_read_request hooks here Yes, with: event::[work]::ap_run_process_connection():..:ap_read_request()::ap_run_pre_read_request()::reqtimeout_before_headers() And then: event::[work]::ap_run_process_connection():..:ap_read_request()::read_request_line()::reqtimeout_filter() event::[work]::ap_run_process_connection():..:ap_read_request()::ap_read_mime_headers()::reqtimeout_filter()* Whereas worker is already here: worker::process_socket()::ap_process_connection():..:ap_read_request()::ap_read_mime_headers()::reqtimeout_filter()* > Process request 2 > Finish request 2 > > So with the event MPM, the pre_read_request hook cannot be used to set > ccfg->in_keep_alive because it's too late. That's just before the very first read about the upcoming request on this kept-alive connection, we must reset the state here, and set in_keep_alive (which really is the state of the connection until we read something by the way, and waiting for this read shall not be accounted). > > May I remind you that I have proposed a working patch? ;-) It can't be correct, as any concurrent read (be it speculative) during the keep-alive state. -- You are receiving this mail because: You are the assignee for the bug. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
