On Mon, May 13, 2024 at 1:32 PM Stefan Eissing via dev <dev@httpd.apache.org> wrote: > > I have merged https://github.com/icing/mod_h2/pull/280 to the mod-h2 on > github. With mpm_event, this will return HTTP/2 connections more often to the > mpm, thus freeing a worker. > > While this sounds good, I am not sure this is beneficial for a server under > load. The current connection state handling is designed for HTTP/1.x where a > connection is "given back" to the MPM at the end of a request. > > AFAICT, the MPM assumes that, once any pending output is written, it may > close the connection under load. Because in HTTP/1.x it means the connection > has served the last response completely. The client should be grateful and > cope well with the connection being closed subsequently due to other clients > demands. > > In HTTP/2 so far, we did return to the MPM only when all requests had been > served. The connection is therefore really in a similar state to HTTP/1.x. > The client has got its responses, we are free to close. > > With the change in PR 280, we return on being flow blocked. The response(s) > are *not* finished. If MPM now closes such a connection under load, the > client will most likely try again. This seems counter productive.
AFAICT the CONN_STATE_WRITE_COMPLETION state is different depending on CONN_SENSE_WANT_READ and CONN_SENSE_WANT_WRITE/DEFAULT. With CONN_SENSE_WANT_WRITE/DEFAULT, mpm_event will indeed assume flush (nonblocking) before close, but with CONN_SENSE_WANT_READ it will poll for read on the connection and come back to the process_connection hooks when something is in. When mod_h2 waits for some WINDOW_UPDATE it wants the MPM to POLLIN (right?), h2_c1_hook_process_connection() should recover/continue with the new data from the client. And since it's a c1 connection I don't think there needs to be some new pollset/queues sizing adjustment to do either, so we should be good? > > Therefore I am hesitant if this change is beneficial for us. It frees up a > worker thread - that is good - but. Do we need a new "connection state" here? > > WDYT? I think the semantics of CONN_STATE_WRITE_COMPLETION with CONN_SENSE_WANT_* are quite unintuitive (for the least), so we probably should have different states for CONN_STATE_WRITE_COMPLETION (flush before close) and CONN_STATE_POLL_READ/POLL_WRITE/POLL_RW which would be how a process_connection hook would return to the MPM just for poll()ing. Regards; Yann.
