> On 4 Jan 2024, at 20:03, Roman Arutyunyan <a...@nginx.com> wrote: > > Hi, > > On Wed, Dec 27, 2023 at 06:34:58PM +0400, Sergey Kandaurov wrote: >> On Wed, Dec 13, 2023 at 06:06:59PM +0400, Roman Arutyunyan wrote: >> >>> # HG changeset patch >>> # User Roman Arutyunyan <a...@nginx.com> >>> # Date 1702476295 -14400 >>> # Wed Dec 13 18:04:55 2023 +0400 >>> # Node ID 844486cdd43a32d10b78493d7e7b80e9e2239d7e >>> # Parent 6c8595b77e667bd58fd28186939ed820f2e55e0e >>> Stream: socket peek in preread phase. >>> >>> Previously, preread buffer was always read out from socket, which made it >>> impossible to terminate SSL on the connection without introducing additional >>> SSL BIOs. The following patches will rely on this. >>> >>> Now, when possible, recv(MSG_PEEK) is used instead, which keeps data in >>> socket. >>> It's called if SSL is not already terminated and if an egde-triggered event >>> method is used. For epoll, EPOLLRDHUP support is also required. >>> >>> diff --git a/src/stream/ngx_stream_core_module.c >>> b/src/stream/ngx_stream_core_module.c >>> --- a/src/stream/ngx_stream_core_module.c >>> +++ b/src/stream/ngx_stream_core_module.c >>> @@ -10,6 +10,10 @@ >>> #include <ngx_stream.h> >>> >>> >>> +static ngx_int_t ngx_stream_preread_peek(ngx_stream_session_t *s, >>> + ngx_stream_phase_handler_t *ph); >>> +static ngx_int_t ngx_stream_preread(ngx_stream_session_t *s, >>> + ngx_stream_phase_handler_t *ph); >>> static ngx_int_t ngx_stream_core_preconfiguration(ngx_conf_t *cf); >>> static void *ngx_stream_core_create_main_conf(ngx_conf_t *cf); >>> static char *ngx_stream_core_init_main_conf(ngx_conf_t *cf, void *conf); >>> @@ -203,8 +207,6 @@ ngx_int_t >>> ngx_stream_core_preread_phase(ngx_stream_session_t *s, >>> ngx_stream_phase_handler_t *ph) >>> { >>> - size_t size; >>> - ssize_t n; >>> ngx_int_t rc; >>> ngx_connection_t *c; >>> ngx_stream_core_srv_conf_t *cscf; >>> @@ -217,56 +219,40 @@ ngx_stream_core_preread_phase(ngx_stream >>> >>> if (c->read->timedout) { >>> rc = NGX_STREAM_OK; >>> + goto done; >>> + } >>> >>> - } else if (c->read->timer_set) { >>> - rc = NGX_AGAIN; >>> + if (!c->read->timer_set) { >>> + rc = ph->handler(s); >>> >>> - } else { >>> - rc = ph->handler(s); >>> + if (rc != NGX_AGAIN) { >>> + goto done; >>> + } >>> } >>> >>> - while (rc == NGX_AGAIN) { >>> - >>> + if (c->buffer == NULL) { >>> + c->buffer = ngx_create_temp_buf(c->pool, >>> cscf->preread_buffer_size); >>> if (c->buffer == NULL) { >>> - c->buffer = ngx_create_temp_buf(c->pool, >>> cscf->preread_buffer_size); >>> - if (c->buffer == NULL) { >>> - rc = NGX_ERROR; >>> - break; >>> - } >>> + rc = NGX_ERROR; >>> + goto done; >>> } >>> - >>> - size = c->buffer->end - c->buffer->last; >>> - >>> - if (size == 0) { >>> - ngx_log_error(NGX_LOG_ERR, c->log, 0, "preread buffer full"); >>> - rc = NGX_STREAM_BAD_REQUEST; >>> - break; >>> - } >>> + } >>> >>> - if (c->read->eof) { >>> - rc = NGX_STREAM_OK; >>> - break; >>> - } >>> - >>> - if (!c->read->ready) { >>> - break; >>> - } >>> - >>> - n = c->recv(c, c->buffer->last, size); >>> + if (c->ssl == NULL >>> + && (ngx_event_flags & NGX_USE_CLEAR_EVENT) >>> + && ((ngx_event_flags & NGX_USE_EPOLL_EVENT) == 0 >>> +#if (NGX_HAVE_EPOLLRDHUP) >>> + || ngx_use_epoll_rdhup >>> +#endif >> >> BTW, c->ssl needs to be guarded under an appropriate macro test. >> Probably, it makes sense to rewrite this in a more readable way. >> For example: >> >> : peak = 0; >> : >> : #if (NGX_HAVE_KQUEUE) >> : if (ngx_event_flags & NGX_USE_KQUEUE_EVENT) { >> : peak = 1; >> : } >> : #endif >> : >> : #if (NGX_HAVE_EPOLLRDHUP) >> : if ((ngx_event_flags & NGX_USE_EPOLL_EVENT) && ngx_use_epoll_rdhup) { >> : peak = 1; >> : } >> : #endif >> : >> : #if (NGX_STREAM_SSL) >> : if (c->ssl) { >> : peak = 0; >> : } >> : #endif > > [..] > > I think it's still too complicated. I suggest a separate function: > > diff --git a/src/stream/ngx_stream_core_module.c > b/src/stream/ngx_stream_core_module.c > --- a/src/stream/ngx_stream_core_module.c > +++ b/src/stream/ngx_stream_core_module.c > @@ -10,6 +10,7 @@ > #include <ngx_stream.h> > > > +static ngx_int_t ngx_stream_preread_can_peek(ngx_connection_t *c); > static ngx_int_t ngx_stream_preread_peek(ngx_stream_session_t *s, > ngx_stream_phase_handler_t *ph); > static ngx_int_t ngx_stream_preread(ngx_stream_session_t *s, > @@ -238,14 +239,7 @@ ngx_stream_core_preread_phase(ngx_stream > } > } > > - if (c->ssl == NULL > - && (ngx_event_flags & NGX_USE_CLEAR_EVENT) > - && ((ngx_event_flags & NGX_USE_EPOLL_EVENT) == 0 > -#if (NGX_HAVE_EPOLLRDHUP) > - || ngx_use_epoll_rdhup > -#endif > - )) > - { > + if (ngx_stream_preread_can_peek(c)) { > rc = ngx_stream_preread_peek(s, ph); > > } else { > @@ -298,6 +292,35 @@ done: > > > static ngx_int_t
ngx_uint_t may be? > +ngx_stream_preread_can_peek(ngx_connection_t *c) > +{ > +#if (NGX_STREAM_SSL) > + if (c->ssl) { > + return 0; > + } > +#endif > + > + if ((ngx_event_flags & NGX_USE_CLEAR_EVENT) == 0) { > + return 0; > + } BTW, the only purpose of this check seems to allow testing level triggered events with epoll/kqueue using --with-cc-opt="-DNGX_HAVE_CLEAR_EVENT=0". > + > +#if (NGX_HAVE_KQUEUE) > + if (ngx_event_flags & NGX_USE_KQUEUE_EVENT) { > + return 1; > + } > +#endif > + > +#if (NGX_HAVE_EPOLLRDHUP) > + if ((ngx_event_flags & NGX_USE_EPOLL_EVENT) && ngx_use_epoll_rdhup) { > + return 1; > + } > +#endif > + > + return 0; > +} > + > + > +static ngx_int_t > ngx_stream_preread_peek(ngx_stream_session_t *s, ngx_stream_phase_handler_t > *ph) > { > ssize_t n; > Looks good. -- Sergey Kandaurov _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel