<div dir="ltr">Great job, the patch works well.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Aug 16, 2019 at 10:24 PM Roman Arutyunyan <<a href="mailto:arut@nginx.com">arut@nginx.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On Wed, Jul 24, 2019 at 11:47:19AM +0800, 洪志道 wrote:<br>
> Hi, I found an issue with the patch.<br>
> <br>
> 1. Reproduce<br>
> (/usr/local/nginx/html/5M.txt, Just making `ngx_http_set_write_handler` be<br>
> called.)<br>
> <br>
> telnet localhost 80<br>
> GET /5M.txt HTTP/1.1<br>
> HOST: 1.1.1.1<br>
> <br>
> -- response--<br>
> <br>
> GET /5M.txt HTTP/1.1<br>
> HOST: 1.1.1.1<br>
> <br>
> --50 error-- /* happened in keepalive */<br>
<br>
Thanks for reporting this.<br>
<br>
> 2. Reason<br>
> It happens in the case of keepalive.<br>
> Since `ngx_http_set_write_handler` is called, c->write is active and ready,<br>
> after receiving http request:<br>
> ngx_http_static_handler is called<br>
> {<br>
> ngx_http_static_send(); // task has been added thread pool, but its<br>
> event is not complete.<br>
> r->write_event_handler = ngx_http_static_write_event_handler;<br>
> }<br>
> And the epoll write event is triggered. So<br>
> ngx_http_static_write_event_handler is called again.<br>
> But task->event.active is true. ngx_thread_task_post() will fail.<br>
> Throws "task #%ui already active".<br>
<br>
The problem only manifests itself with epoll. After both EPOLLIN and EPOLLOUT<br>
for the same socket are registered in epoll, EPOLLOUT is reported along with<br>
EPOLLIN despite the fact that only the read state of the socket changed but the<br>
write state didn't (socket was and is writable). This is not a problem by<br>
itself, but it helped to find the problem with this patch.<br>
<br>
While thread task is in progress, calling ngx_http_static_write_event_handler()<br>
resulted in posting the task again, which produced the error.<br>
<br>
> 3. Early patch.<br>
> ```<br>
> diff -r bd11e3399021 src/http/modules/ngx_http_static_module.c<br>
> --- a/src/http/modules/ngx_http_static_module.c Tue Oct 30 15:00:49 2018<br>
> +0300<br>
> +++ b/src/http/modules/ngx_http_static_module.c Tue Jul 23 23:46:22 2019<br>
> -0400<br>
> @@ -318,6 +318,12 @@ ngx_http_static_write_event_handler(ngx_<br>
> {<br>
> ngx_int_t rc;<br>
> <br>
> + if (r->open_file_info.thread_task != NULL<br>
> + && r->open_file_info.thread_task->event.complete == 0)<br>
> + {<br>
> + return;<br>
> + }<br>
> +<br>
> rc = ngx_http_static_send(r);<br>
> <br>
> if (rc != NGX_DONE) {<br>
<br>
Similar problem in thread read is prevented by checking the flag r->aio.<br>
I did the same for thread open (see attachment).<br>
<br>
> ```<br>
> <br>
> Thanks.<br>
> <br>
> <br>
> On Mon, Jul 22, 2019 at 10:05 PM Roman Arutyunyan <<a href="mailto:arut@nginx.com" target="_blank">arut@nginx.com</a>> wrote:<br>
> <br>
> > Hi,<br>
> ><br>
> > On Sat, Jul 20, 2019 at 09:44:25AM +0800, 洪志道 wrote:<br>
> > > > The patch wasn't updated since that but I suspect it could be still<br>
> > > applied to nginx, maybe with some minor tweaks.<br>
> > ><br>
> > > We still appreciate your work.<br>
> > ><br>
> > > BTW, are the patches in the attachment up to date?<br>
> > > If not, can you share the latest patch?<br>
> ><br>
> > We had no patches since november 1, 2018. The last patch is in this<br>
> > message:<br>
> ><br>
> > <a href="http://mailman.nginx.org/pipermail/nginx-devel/2018-November/011538.html" rel="noreferrer" target="_blank">http://mailman.nginx.org/pipermail/nginx-devel/2018-November/011538.html</a><br>
> ><br>
> > > We are happy to apply it and feedback if possible.<br>
> > > When there are many static files and accesses, do you think it will works<br>
> > > very well?<br>
> ><br>
> > We didn't have much feedback so far. So we would definitely appreciate<br>
> > your<br>
> > contribution to testing it.<br>
> ><br>
> > > On Sat, Jul 20, 2019 at 2:42 AM Maxim Konovalov <<a href="mailto:maxim@nginx.com" target="_blank">maxim@nginx.com</a>> wrote:<br>
> > ><br>
> > > > Hi hongzhidao,<br>
> > > ><br>
> > > > The patch wasn't merged as we didn't see much interest and real<br>
> > > > technical feedback from potential testers.<br>
> > > ><br>
> > > > The code adds additional complexity to the nginx core with all<br>
> > > > associated costs of maintaining the code virtually forever. In the<br>
> > > > same time at this point it brings no measurable value to the<br>
> > > > community. At least, we haven't seen any proofs that it does.<br>
> > > ><br>
> > > > The patch wasn't updated since that but I suspect it could be still<br>
> > > > applied to nginx, maybe with some minor tweaks.<br>
> > > ><br>
> > > > Maxim<br>
> > > ><br>
> > > > On 19/07/2019 18:26, 洪志道 wrote:<br>
> > > > > Hi.<br>
> > > > > Will this patch be merged into the main branch?<br>
> > > > > What is the latest patch? We can help with the test.<br>
> > > > > Thanks.<br>
> > > > ><br>
> > > > > On Sat, Feb 9, 2019 at 6:40 AM Ka-Hing Cheung via nginx-devel<br>
> > > > > <<a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a> <mailto:<a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a>>> wrote:<br>
> > > > ><br>
> > > > > Unfortunately our test colo is not setup to do performance<br>
> > testing<br>
> > > > > (the traffic it receives varies too much). We do intend to merge<br>
> > > > > this<br>
> > > > > to our production colos but there's no timeline yet.<br>
> > > > ><br>
> > > > > Yuchen (CC'ed) will be the main contact from now on as today is<br>
> > my<br>
> > > > > last day at Cloudflare.<br>
> > > > ><br>
> > > > > - Ka-Hing<br>
> > > > ><br>
> > > > > On Thu, Feb 7, 2019 at 5:39 AM Maxim Konovalov <<a href="mailto:maxim@nginx.com" target="_blank">maxim@nginx.com</a><br>
> > > > > <mailto:<a href="mailto:maxim@nginx.com" target="_blank">maxim@nginx.com</a>>> wrote:<br>
> > > > > ><br>
> > > > > > Great. Thanks for the testing!<br>
> > > > > ><br>
> > > > > > Did you see any measurable perf. metrics changes comparing to<br>
> > your<br>
> > > > > > aio open implementation or comparing to nginx without aio open<br>
> > > > > support?<br>
> > > > > ><br>
> > > > > > We are still waiting for additional input from another tester,<br>
> > who<br>
> > > > > > expressed interest before.<br>
> > > > > ><br>
> > > > > > Thanks,<br>
> > > > > ><br>
> > > > > > Maxim<br>
> > > > > ><br>
> > > > > > On 07/02/2019 00:19, Ka-Hing Cheung wrote:<br>
> > > > > > > This has been running in our test colo for the past week<br>
> > > > > with no ill effects.<br>
> > > > > > ><br>
> > > > > > > On Wed, Jan 23, 2019 at 4:39 AM Maxim Konovalov<br>
> > > > > <<a href="mailto:maxim@nginx.com" target="_blank">maxim@nginx.com</a> <mailto:<a href="mailto:maxim@nginx.com" target="_blank">maxim@nginx.com</a>>> wrote:<br>
> > > > > > >><br>
> > > > > > >> Hi Ka-Hing,<br>
> > > > > > >><br>
> > > > > > >> Roman told me that the delta is because of your changes.<br>
> > > > > > >><br>
> > > > > > >> Thanks for your time on that. Waiting for your testing<br>
> > > > > results.<br>
> > > > > > >><br>
> > > > > > >> Maxim<br>
> > > > > > >><br>
> > > > > > >> On 22/01/2019 22:34, Ka-Hing Cheung via nginx-devel wrote:<br>
> > > > > > >>> I spoke too soon, just realized our test colo is running<br>
> > > > > the patches<br>
> > > > > > >>> without aio_open on. Flipping that switch now.<br>
> > > > > > >>><br>
> > > > > > >>> Also, including the patch that we applied on top in<br>
> > > > > addition to the<br>
> > > > > > >>> massaging we did to resolve conflicts. I haven't dug too<br>
> > > > > deep to see<br>
> > > > > > >>> if stock nginx also requires similar changes or they are<br>
> > only<br>
> > > > > > >>> necessary because of our other nginx changes:<br>
> > > > > > >>><br>
> > > > > > >> [...]<br>
> > > > > > >><br>
> > > > > > >> --<br>
> > > > > > >> Maxim Konovalov<br>
> > > > > ><br>
> > > > > ><br>
> > > > > > --<br>
> > > > > > Maxim Konovalov<br>
> > > > > _______________________________________________<br>
> > > > > nginx-devel mailing list<br>
> > > > > <a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a> <mailto:<a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a>><br>
> > > > > <a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" rel="noreferrer" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
> > > > ><br>
> > > > ><br>
> > > > > _______________________________________________<br>
> > > > > nginx-devel mailing list<br>
> > > > > <a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
> > > > > <a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" rel="noreferrer" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
> > > > ><br>
> > > ><br>
> > > ><br>
> > > > --<br>
> > > > Maxim Konovalov<br>
> > > ><br>
> ><br>
> > > _______________________________________________<br>
> > > nginx-devel mailing list<br>
> > > <a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
> > > <a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" rel="noreferrer" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
> ><br>
> ><br>
> > --<br>
> > Roman Arutyunyan<br>
> > _______________________________________________<br>
> > nginx-devel mailing list<br>
> > <a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
> > <a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" rel="noreferrer" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
<br>
> _______________________________________________<br>
> nginx-devel mailing list<br>
> <a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
> <a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" rel="noreferrer" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
<br>
<br>
-- <br>
Roman Arutyunyan<br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" rel="noreferrer" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a></blockquote></div>