<div dir="ltr">Hello,<br><br>Thank you for the very detailed feedback.<br><br>A couple of mistakes from my side, when submitting the patch:<br><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Also, this looks irrelevant to the issue being addressed.</blockquote><br>Yep, this is not relevant to the issue being addressed, i think i added this<br>change here by mistake, my bad.<br><br><br>Also, i missed to add another part of the patch, which removes this block (the<br>original call to ngx_write_chain_to_temp_file()):<br><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> n = ngx_write_chain_to_temp_file(p->temp_file, out);</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br> if (n == NGX_ERROR) {<br> p->cache_write_fail = 1;<br> return NGX_ABORT;<br> } </blockquote><br><br>I'm guessing you were referring to that block in this comment: (?)<br><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">- In case of success, the following code will write the data<br> again, resulting in file corruption and likely various other<br> issues.</blockquote><br><br>Honestly, all the ins and outs of the function<br>ngx_event_pipe_write_chain_to_temp_file() are not quite clear to me. But with<br>this change the issue of closing the client connection abruptly without<br>finishing to serve the response (when proxy cache failed to write to disk) goes<br>away (at least in my tests).<br>However, i surely cannot guarantee that nothing else will break :)<br>(but fwiw i did run the nginx-tests suite with this change, and all tests pass)<br><br>I will do more testing and try to analyze what exactly is happening in this<br>function.<br><br>Thank you.<br><br>P.S. if i could ask - what does the p->cacheable flag mean? The ngx_event_pipe_t<br>struct is not documented in <a href="http://nginx.org/en/docs/dev/development_guide.html">http://nginx.org/en/docs/dev/development_guide.html</a><br>and it is kinda hard to understand all the pipes/buffers and the buffer<br>shadowing mechanism/purpose just from reading the code :)<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 3, 2022 at 12:02 AM Maxim Dounin <<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</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">Hello!<br>
<br>
On Mon, May 02, 2022 at 04:36:09PM +0300, Kipras Mancevicius wrote:<br>
<br>
> # HG changeset patch<br>
> # User Kipras Mancevicius <<a href="mailto:kiprasm@gmail.com" target="_blank">kiprasm@gmail.com</a>><br>
> # Date 1651495240 -10800<br>
> # Mon May 02 15:40:40 2022 +0300<br>
> # Branch proxy_cache_write_disk_full_fix<br>
> # Node ID e6e708fd7958987027f6de7748d948e5015ed32b<br>
> # Parent a736a7a613ea6e182ff86fbadcb98bb0f8891c0b<br>
> Do not break downstream connection on proxy_cache write error<br>
> <br>
> Continue sending response to downstream connection and delete temp cache file on<br>
> upstream connection finalization.<br>
> <br>
> diff -r a736a7a613ea -r e6e708fd7958 src/core/ngx_file.c<br>
> --- a/src/core/ngx_file.c Tue Feb 08 17:35:27 2022 +0300<br>
> +++ b/src/core/ngx_file.c Mon May 02 15:40:40 2022 +0300<br>
> @@ -765,6 +765,13 @@<br>
> ngx_delete_file_n " \"%s\" failed", name);<br>
> <br>
> }<br>
> + } else {<br>
> + /*<br>
> + * if the file failed to fully copy (e.g. target disk is full) - it<br>
> + * may remain (partially written) in the target disk. Attempt to<br>
> + * delete it.<br>
> + */<br>
> + ngx_delete_file(name);<br>
<br>
Trying to delete a file "just in case" looks clearly wrong. If at <br>
all, this is to be done in ngx_copy_file(), the function which <br>
created the file and knows what in fact happened. Further, it is <br>
not clear if it's to be done at all: in many practical cases an <br>
incomplete file is better than no file at all, and this is a <br>
generic function.<br>
<br>
Also, this looks irrelevant to the issue being addressed.<br>
<br>
> }<br>
> <br>
> ngx_free(name);<br>
> diff -r a736a7a613ea -r e6e708fd7958 src/event/ngx_event_pipe.c<br>
> --- a/src/event/ngx_event_pipe.c Tue Feb 08 17:35:27 2022 +0300<br>
> +++ b/src/event/ngx_event_pipe.c Mon May 02 15:40:40 2022 +0300<br>
> @@ -753,10 +753,21 @@<br>
> out = p->writing;<br>
> p->writing = NULL;<br>
> <br>
> - n = ngx_write_chain_to_temp_file(p->temp_file, NULL);<br>
> + /*<br>
> + * stop writing to cache on cache write fail, but keep sending response<br>
> + * downstream. Incomplete temp cache file will be deleted later.<br>
> + */<br>
> + if (p->cache_write_fail) {<br>
> + return NGX_BUSY;<br>
> + } else {<br>
> + n = ngx_write_chain_to_temp_file(p->temp_file, NULL);<br>
<br>
This looks terribly wrong: if this code will happen actually <br>
return NGX_BUSY, a thread-based operation won't be completed, <br>
potentially resulting in a connection hang and/or a socket leak.<br>
<br>
> <br>
> - if (n == NGX_ERROR) {<br>
> - return NGX_ABORT;<br>
> + if (n == NGX_ERROR) {<br>
> + p->cache_write_fail = 1;<br>
> + p->cacheable = 0;<br>
> +<br>
> + return NGX_BUSY;<br>
<br>
This looks wrong: if an error happens as a result of a <br>
thread-based operation, it is incorrect to just ignore this. Data <br>
being written needs to be recovered somehow.<br>
<br>
> + }<br>
> }<br>
> <br>
> goto done;<br>
> @@ -777,6 +788,23 @@<br>
> out = p->in;<br>
> }<br>
> <br>
> + /*<br>
> + * stop writing to cache on cache write fail, but keep sending response<br>
> + * downstream. Incomplete temp cache file will be deleted later.<br>
> + */<br>
> + if (p->cache_write_fail) {<br>
> + return NGX_BUSY;<br>
> + } else {<br>
> + n = ngx_write_chain_to_temp_file(p->temp_file, out);<br>
> +<br>
> + if (n == NGX_ERROR) {<br>
> + p->cache_write_fail = 1;<br>
> + p->cacheable = 0;<br>
> +<br>
> + return NGX_BUSY;<br>
> + }<br>
> + }<br>
> +<br>
<br>
This also looks wrong:<br>
<br>
- The code tries to write everything in out, ignoring things like <br>
temp_file_write_size and max_temp_file_size.<br>
<br>
- In case of success, the following code will write the data <br>
again, resulting in file corruption and likely various other <br>
issues.<br>
<br>
Further, the following writes (which are the only writes <br>
meaningful for the current request, as you don't try to update <br>
p->out) are not really protected by any error checking, so the <br>
initial objective of the patch does not seem to be achieved.<br>
<br>
Please also note that the ticket in question says "without <br>
introducing unreasonable complexity in the source code" for a <br>
reason.<br>
<br>
Hope this helps.<br>
<br>
-- <br>
Maxim Dounin<br>
<a href="http://mdounin.ru/" rel="noreferrer" target="_blank">http://mdounin.ru/</a><br>
_______________________________________________<br>
nginx-devel mailing list -- <a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
To unsubscribe send an email to <a href="mailto:nginx-devel-leave@nginx.org" target="_blank">nginx-devel-leave@nginx.org</a><br>
</blockquote></div>