Hello! On Mon, Nov 18, 2019 at 08:05:39PM +0100, Sergey Brester wrote:
> 18.11.2019 18:11, Maxim Dounin wrote: > > > Hello! > > > > On Mon, Nov 18, 2019 at 05:48:46PM +0100, Sergey Brester wrote: > > > >> Looks like [efd71d49bde0 [2]] could be indeed responsible for that: I see > >> at least one state where rev->ready could remain 1 (after rev->available > >> gets 0) e. g. deviation between blocks [efd71d49bde0#l10.8 [3]] and > >> [efd71d49bde0#l11.8 [4]] where first did not reset rev->ready and for > >> example if ngx_socket_nread in [efd71d49bde0#l10.38 [5]] would write 0 > >> into rev->available, so rev->ready remains 1 yet. > > > > There is no deviation here. The rev->available field holds the > > number of bytes available for reading (or -1 if it's not known), > > while rev->ready indicates if reading is possible. The reading > > can be possible even WHEN REV->AVAILABLE IS ZERO - notably, when > > there is a potential pending EOF. > > Sure, > but I told about the case where rev->ready indicates the READING IS > POSSIBLE, > but REV->AVAILABLE IS LESS AS ZERO. > > If you mean this is to noglect, then it is ok, but somehow it looks > backwards incompatible to me. > At least other async-IO procedures (for example KQUEUE [1]) handle this > differently. The code you are referring to defines how nginx behaves when rev->available becomes zero or negative after it is decremented by the amount of bytes read. In case of kqueue, rev->available is set to 0 to indicate there are no data to read, and rev->ready is also set to 0 unless pending EOF is indicated by kqueue. In case of unspecified event methods, we cannot rely on the pending EOF indication (as this indication is only available with kqueue and with epoll with recent enough kernels). As such, we can only reset rev->ready when rev->available becomes negative (and then set to 0), but not when rev->available becomes exactly zero. > Anyway I saw never REV->AVAILABLE LESS AS ZERO, if rev->ready got 1 > until now. This is explained in the commit log of the revant commit. Quoting it here: : With other event methods rev->available is now set to -1 when the socket : is ready for reading. Later in ngx_recv() and ngx_recv_chain(), if : full buffer is received, real number of bytes in the socket buffer is : retrieved using ioctl(FIONREAD). Reading more than this number of bytes : ensures that even with edge-triggered event methods the event will be : triggered again, so it is safe to stop processing of the socket and : switch to other connections. Further, it is now explicitly documented in the relevant comment in src/event/ngx_event.h, see here: http://hg.nginx.org/nginx/rev/efd71d49bde0#l9.1 The rev->available set to -1 now indicates that there are unspecified amount of bytes to read. Previously rev->available was never negative (and cannot be negative on most platforms, given that it was a single bit field), and certainly this is a change. And in theory it can cause problems if some code relies on the previous semantics of rev->available being 0 or 1 with epoll, and checks for "rev->available == 1" for some reason (tests for "rev->available", "!rev->available" and "rev->available == 0" will work correctly without modifications). But in practice I wouldn't expect this change to cause any problems with properly written code. As already outlined in my reply to the original message, the reason for the segmentation faults observed seems to be failure of the upload module to block events after reading the request body. -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
