Re: Detecting client aborts and stream resets
Zitat von Stefan Eissing: Thanks for the patch! I applied it to trunk in r1743335, will be part of next 1.5.4 release. I only omitted the last change as I do not want to set aborted on the main connection every time the session closes. Ok, that's fine for me. Thanks a lot Stefan! Regards, Michael
Re: Detecting client aborts and stream resets
Thanks for the patch! I applied it to trunk in r1743335, will be part of next 1.5.4 release. I only omitted the last change as I do not want to set aborted on the main connection every time the session closes. Cheers, Stefan > Am 10.05.2016 um 14:37 schrieb Michael Kaufmann: > > Zitat von William A Rowe Jr : > >> On Wed, May 4, 2016 at 3:44 PM, Michael Kaufmann >> wrote: >> >>> William is right, this is not a good idea. The ->aborted flag should serve this purpose of telling anyone interested that this connection is not longer delivering. I will make a github release soon where that is working and you can test. >>> Thank you Stefan! It is now working for stream resets, but it's not yet >>> working if the client closes the whole TCP connection. >>> >> >> As expected... this is why I pointed out in my first reply that you don't >> want a single-protocol solution to this puzzle. > > Of course I'd also prefer a general solution. > > I have created a patch for mod_http2: With the patch, it sets c->aborted on > the master connection and also on the "dummy" connections (streams) when the > client closes the TCP connection. > > @Stefan: It would be great if you could check this code and add it to > mod_http2 :-) > > > Index: modules/http2/h2_mplx.c > === > --- modules/http2/h2_mplx.c (revision 1743146) > +++ modules/http2/h2_mplx.c (working copy) > @@ -488,6 +488,15 @@ > return 1; > } > > +static int task_abort_connection(void *ctx, void *val) > +{ > +h2_task *task = val; > +if (task->c) { > +task->c->aborted = 1; > +} > +return 1; > +} > + > apr_status_t h2_mplx_release_and_join(h2_mplx *m, apr_thread_cond_t *wait) > { > apr_status_t status; > @@ -537,6 +546,8 @@ > * and workers *should* return in a timely fashion. > */ > for (i = 0; m->workers_busy > 0; ++i) { > +h2_ihash_iter(m->tasks, task_abort_connection, m); > + > m->join_wait = wait; > status = apr_thread_cond_timedwait(wait, m->lock, > apr_time_from_sec(wait_secs)); > > @@ -591,6 +602,7 @@ > AP_DEBUG_ASSERT(m); > if (!m->aborted && enter_mutex(m, ) == APR_SUCCESS) { > m->aborted = 1; > +m->c->aborted = 1; > h2_ngn_shed_abort(m->ngn_shed); > leave_mutex(m, acquired); > } > > > > >> See my later reply about detecting connection tear-down, patches welcome. > > Sounds good! If someone sends a patch, I will help to test it.
Re: Detecting client aborts and stream resets
Zitat von William A Rowe Jr: On Wed, May 4, 2016 at 3:44 PM, Michael Kaufmann wrote: William is right, this is not a good idea. The ->aborted flag should serve this purpose of telling anyone interested that this connection is not longer delivering. I will make a github release soon where that is working and you can test. Thank you Stefan! It is now working for stream resets, but it's not yet working if the client closes the whole TCP connection. As expected... this is why I pointed out in my first reply that you don't want a single-protocol solution to this puzzle. Of course I'd also prefer a general solution. I have created a patch for mod_http2: With the patch, it sets c->aborted on the master connection and also on the "dummy" connections (streams) when the client closes the TCP connection. @Stefan: It would be great if you could check this code and add it to mod_http2 :-) Index: modules/http2/h2_mplx.c === --- modules/http2/h2_mplx.c (revision 1743146) +++ modules/http2/h2_mplx.c (working copy) @@ -488,6 +488,15 @@ return 1; } +static int task_abort_connection(void *ctx, void *val) +{ +h2_task *task = val; +if (task->c) { +task->c->aborted = 1; +} +return 1; +} + apr_status_t h2_mplx_release_and_join(h2_mplx *m, apr_thread_cond_t *wait) { apr_status_t status; @@ -537,6 +546,8 @@ * and workers *should* return in a timely fashion. */ for (i = 0; m->workers_busy > 0; ++i) { +h2_ihash_iter(m->tasks, task_abort_connection, m); + m->join_wait = wait; status = apr_thread_cond_timedwait(wait, m->lock, apr_time_from_sec(wait_secs)); @@ -591,6 +602,7 @@ AP_DEBUG_ASSERT(m); if (!m->aborted && enter_mutex(m, ) == APR_SUCCESS) { m->aborted = 1; +m->c->aborted = 1; h2_ngn_shed_abort(m->ngn_shed); leave_mutex(m, acquired); } See my later reply about detecting connection tear-down, patches welcome. Sounds good! If someone sends a patch, I will help to test it.
Re: Detecting client aborts and stream resets
On Wed, May 4, 2016 at 3:44 PM, Michael Kaufmannwrote: > William is right, this is not a good idea. The ->aborted flag should serve >> this purpose of telling anyone interested that this connection is not >> longer delivering. I will make a github release soon where that is working >> and you can test. >> >> > Thank you Stefan! It is now working for stream resets, but it's not yet > working if the client closes the whole TCP connection. > As expected... this is why I pointed out in my first reply that you don't want a single-protocol solution to this puzzle. See my later reply about detecting connection tear-down, patches welcome.
Re: Detecting client aborts and stream resets
William is right, this is not a good idea. The ->aborted flag should serve this purpose of telling anyone interested that this connection is not longer delivering. I will make a github release soon where that is working and you can test. Thank you Stefan! It is now working for stream resets, but it's not yet working if the client closes the whole TCP connection. Regards, Michael
Re: Detecting client aborts and stream resets
On Tue, May 3, 2016 at 2:05 PM, Michael Kaufmannwrote: > Zitat von William A Rowe Jr : > >> >> Nope - an optional function in mod_http2 is too special case, generators >> must remain protocol (socket or other transport) agnostic. >> > > Sure, official Apache modules should not call protocol-dependent hooks, > but it could be a solution for 3rd party modules. Nonsense, third party module designers should be equally aware - when you start meddling in unexpected edge cases, binary ABI won't be maintained in those edge cases. You are always better off relying on the intended design of httpd, unless you want to chase a moving target. Extending this detection with a protocol-agnostic API would be interesting, however an optional function has only a single registered provider, so it would be the wrong interface. You want an interface that would test/report "gone away" irrespective of http1 vs http2 vs future protocols. > In the case of mod_cache'd content, the generator can't quit, it is already >> populating a cache, which means you'll generate an invalid cached object. >> >> But if you knew that the cache module wasn't collecting info, >> r->c->aborted >> tells you if anyone is still listening, right? >> > > Unfortunately not. While the content generator is running (just preparing > the response, it is not reading from the input filters anymore and not > writing to the output filters yet), Apache does not check the connection's > state. > Good point... where/how would we address this for CPU intensive generators? We certainly don't bother in proxy, because proxy is much lighter-weight. The obvious answer to me in a worker or event MPM would be for the user to make a 'notify close?' request to the MPM, which would add the socket with for the POLLHUP event alone after reading the input brigade (the read will signal r->c->aborted already), with the single goal of setting the flag r->c->aborted upon socket close. Very lightweight, but that presumes that a second poll on the same fd for different events (e.g. poll on read or write) elsewhere won't give the kernel conniptions. mod_http2 has enough going on the master connection that detecting the stream close should occur fairly promptly.
Re: Detecting client aborts and stream resets
> Am 03.05.2016 um 17:35 schrieb William A Rowe Jr: > > On Tue, May 3, 2016 at 9:31 AM, Michael Kaufmann > wrote: > Hi all, > > a content generator module can detect client aborts and stream resets while > it reads the request body. But how can it detect this afterwards, while the > response is being generated? > > This is important for HTTP/2, because the client may reset a stream, and > mod_http2 needs to wait for the content generator to finish. Therefore the > content generator should stop generating the response when it is no longer > needed. > > Is there any API for this? The "conn_rec->aborted" flag exists, but which > Apache function sets this flag? conn_rec->aborted is currently not set by mod_http2 on slave connections, but should. I'll add that. > If there is no API, maybe an optional function for mod_http2 would be a > solution. > > Nope - an optional function in mod_http2 is too special case, generators > must remain protocol (socket or other transport) agnostic. > > In the case of mod_cache'd content, the generator can't quit, it is already > populating a cache, which means you'll generate an invalid cached object. > > But if you knew that the cache module wasn't collecting info, r->c->aborted > tells you if anyone is still listening, right? William is right, this is not a good idea. The ->aborted flag should serve this purpose of telling anyone interested that this connection is not longer delivering. I will make a github release soon where that is working and you can test. -Stefan
Re: Detecting client aborts and stream resets
Zitat von William A Rowe Jr: On Tue, May 3, 2016 at 9:31 AM, Michael Kaufmann wrote: Hi all, a content generator module can detect client aborts and stream resets while it reads the request body. But how can it detect this afterwards, while the response is being generated? This is important for HTTP/2, because the client may reset a stream, and mod_http2 needs to wait for the content generator to finish. Therefore the content generator should stop generating the response when it is no longer needed. Is there any API for this? The "conn_rec->aborted" flag exists, but which Apache function sets this flag? If there is no API, maybe an optional function for mod_http2 would be a solution. Nope - an optional function in mod_http2 is too special case, generators must remain protocol (socket or other transport) agnostic. Sure, official Apache modules should not call protocol-dependent hooks, but it could be a solution for 3rd party modules. In the case of mod_cache'd content, the generator can't quit, it is already populating a cache, which means you'll generate an invalid cached object. But if you knew that the cache module wasn't collecting info, r->c->aborted tells you if anyone is still listening, right? Unfortunately not. While the content generator is running (just preparing the response, it is not reading from the input filters anymore and not writing to the output filters yet), Apache does not check the connection's state. I'm not sure how mod_proxy deals with this - does it abort the backend request when the client closes the connection?
Re: Detecting client aborts and stream resets
Hello Michael. On 5/3/2016 7:31 AM, Michael Kaufmann wrote: Hi all, a content generator module can detect client aborts and stream resets while it reads the request body. But how can it detect this afterwards, while the response is being generated? This is important for HTTP/2, because the client may reset a stream, and mod_http2 needs to wait for the content generator to finish. Therefore the content generator should stop generating the response when it is no longer needed. Is there any API for this? The "conn_rec->aborted" flag exists, but which Apache function sets this flag? The conn_rec->aborted flag is set in the following functions: - ap_core_output_filter() in server/core_filters.c - ap_process_connection() in server/connection.c - process_socket() in server/mpm/event/event.c - worker_main() in server/mpm/winnt/child.c If there is no API, maybe an optional function for mod_http2 would be a solution. Regards, Michael
Re: Detecting client aborts and stream resets
On Tue, May 3, 2016 at 9:31 AM, Michael Kaufmannwrote: > Hi all, > > a content generator module can detect client aborts and stream resets > while it reads the request body. But how can it detect this afterwards, > while the response is being generated? > > This is important for HTTP/2, because the client may reset a stream, and > mod_http2 needs to wait for the content generator to finish. Therefore the > content generator should stop generating the response when it is no longer > needed. > > Is there any API for this? The "conn_rec->aborted" flag exists, but which > Apache function sets this flag? > > If there is no API, maybe an optional function for mod_http2 would be a > solution. > Nope - an optional function in mod_http2 is too special case, generators must remain protocol (socket or other transport) agnostic. In the case of mod_cache'd content, the generator can't quit, it is already populating a cache, which means you'll generate an invalid cached object. But if you knew that the cache module wasn't collecting info, r->c->aborted tells you if anyone is still listening, right?