On Sat, Jun 04, 2022 at 12:13:05AM +0300, Maxim Dounin wrote: > Hello! > > On Thu, Jun 02, 2022 at 03:20:54AM +0000, Aleksei Bavshin via nginx-devel > wrote: > > > > I don't think it's the way to go. Semantically, flush and > > > last_buf are mutually exclusive: last_buf implies flushing of all > > > remaining data, and there should be no flush set if last_buf is > > > set. > > > > > > The "cl->buf->flush = !src->read->eof" approach might be actually > > > better and more obvious: it simply ensures that last_buf and flush > > > are not set at the same time. I don't immediately see any cases > > > where it can break things, and this should fix the original > > > problem with empty UDP packets being sent. > > > > Thanks for confirming that, I wasn't sure it is the right approach exactly > > because I had doubts regarding the mutual exclusivity of the flags. > > Updated patch below; tests are passing on both branches and all systems we > > have in CI. > > > > # HG changeset patch > > # User Aleksei Bavshin <[email protected]> > > # Date 1653330584 25200 > > # Mon May 23 11:29:44 2022 -0700 > > # Node ID 40926829be83986a3a5a5f941d2014000a0acd2e > > # Parent e0cfab501dd11fdd23ad492419692269d3a01fc7 > > Stream: don't flush empty buffers created for read errors. > > > > When we generate the last_buf buffer for an UDP upstream recv error, it does > > not contain any data from the wire. ngx_stream_write_filter attempts to > > forward > > it anyways, which is incorrect (e.g., UDP upstream ECONNREFUSED will be > > translated to an empty packet). > > > > This happens because we mark the buffer as both 'flush' and 'last_buf', and > > ngx_stream_write_filter has special handling for flush with certain types of > > connections (see d127837c714f, 32b0ba4855a6). The flags are meant to be > > mutually exclusive, so the fix is to ensure that flush and last_buf are not > > set > > at the same time. > > > > Reproduction: > > > > stream { > > upstream unreachable { > > server 127.0.0.1:8880; > > } > > server { > > listen 127.0.0.1:8998 udp; > > proxy_pass unreachable; > > } > > } > > > > 1 0.000000000 127.0.0.1 → 127.0.0.1 UDP 47 45588 → 8998 Len=5 > > 2 0.000166300 127.0.0.1 → 127.0.0.1 UDP 47 51149 → 8880 Len=5 > > 3 0.000172600 127.0.0.1 → 127.0.0.1 ICMP 75 Destination unreachable > > (Port > > unreachable) > > 4 0.000202400 127.0.0.1 → 127.0.0.1 UDP 42 8998 → 45588 Len=0 > > > > Fixes d127837c714f. > > > > diff --git a/src/stream/ngx_stream_proxy_module.c > > b/src/stream/ngx_stream_proxy_module.c > > --- a/src/stream/ngx_stream_proxy_module.c > > +++ b/src/stream/ngx_stream_proxy_module.c > > @@ -1735,7 +1735,7 @@ ngx_stream_proxy_process(ngx_stream_sess > > > > cl->buf->temporary = (n ? 1 : 0); > > cl->buf->last_buf = src->read->eof; > > - cl->buf->flush = 1; > > + cl->buf->flush = !src->read->eof; > > > > (*packets)++; > > *received += n; > > Looks good to me. > > Roman, please take another look.
Good to me too. This version is equivalent to the original patch for UDP. The only difference is a side-effect for TCP. Now flush is not set in the last buffer. > > -- > Maxim Dounin > http://mdounin.ru/ > _______________________________________________ > nginx-devel mailing list -- [email protected] > To unsubscribe send an email to [email protected] _______________________________________________ nginx-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
