[PATCH 1 of 3] Stream: socket peek in preread phase
Roman Arutyunyan
arut at nginx.com
Thu Jan 18 15:06:06 UTC 2024
Hi,
On Thu, Jan 18, 2024 at 05:43:08PM +0400, Sergey Kandaurov wrote:
>
> > On 4 Jan 2024, at 20:03, Roman Arutyunyan <arut at 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 <arut at 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?
Yes, indeed.
> > +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".
Sure, both of them can work in level-triggered mode.
> > +
> > +#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 at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
Final version attached.
--
Roman Arutyunyan
-------------- next part --------------
# HG changeset patch
# User Roman Arutyunyan <arut at nginx.com>
# Date 1702476295 -14400
# Wed Dec 13 18:04:55 2023 +0400
# Node ID 7324e8e73595c3093fcc2cbd2b5d6b1a947be3b0
# Parent ee40e2b1d0833b46128a357fbc84c6e23be9be07
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,11 @@
#include <ngx_stream.h>
+static ngx_uint_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,
+ 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 +208,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 +220,33 @@ 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 (ngx_stream_preread_can_peek(c)) {
+ rc = ngx_stream_preread_peek(s, ph);
- if (n == NGX_ERROR || n == 0) {
- rc = NGX_STREAM_OK;
- break;
- }
+ } else {
+ rc = ngx_stream_preread(s, ph);
+ }
- if (n == NGX_AGAIN) {
- break;
- }
-
- c->buffer->last += n;
-
- rc = ph->handler(s);
- }
+done:
if (rc == NGX_AGAIN) {
if (ngx_handle_read_event(c->read, 0) != NGX_OK) {
@@ -311,6 +291,129 @@ ngx_stream_core_preread_phase(ngx_stream
}
+static ngx_uint_t
+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;
+ }
+
+#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;
+ ngx_int_t rc;
+ ngx_err_t err;
+ ngx_connection_t *c;
+
+ c = s->connection;
+
+ n = recv(c->fd, (char *) c->buffer->last,
+ c->buffer->end - c->buffer->last, MSG_PEEK);
+
+ err = ngx_socket_errno;
+
+ ngx_log_debug1(NGX_LOG_DEBUG_STREAM, c->log, 0, "stream recv(): %z", n);
+
+ if (n == -1) {
+ if (err == NGX_EAGAIN) {
+ c->read->ready = 0;
+ return NGX_AGAIN;
+ }
+
+ ngx_connection_error(c, err, "recv() failed");
+ return NGX_STREAM_OK;
+ }
+
+ if (n == 0) {
+ return NGX_STREAM_OK;
+ }
+
+ c->buffer->last += n;
+
+ rc = ph->handler(s);
+
+ if (rc != NGX_AGAIN) {
+ c->buffer->last = c->buffer->pos;
+ return rc;
+ }
+
+ if (c->buffer->last == c->buffer->end) {
+ ngx_log_error(NGX_LOG_ERR, c->log, 0, "preread buffer full");
+ return NGX_STREAM_BAD_REQUEST;
+ }
+
+ if (c->read->pending_eof) {
+ return NGX_STREAM_OK;
+ }
+
+ c->buffer->last = c->buffer->pos;
+
+ return NGX_AGAIN;
+}
+
+
+static ngx_int_t
+ngx_stream_preread(ngx_stream_session_t *s, ngx_stream_phase_handler_t *ph)
+{
+ ssize_t n;
+ ngx_int_t rc;
+ ngx_connection_t *c;
+
+ c = s->connection;
+
+ while (c->read->ready) {
+
+ n = c->recv(c, c->buffer->last, c->buffer->end - c->buffer->last);
+
+ if (n == NGX_AGAIN) {
+ return NGX_AGAIN;
+ }
+
+ if (n == NGX_ERROR || n == 0) {
+ return NGX_STREAM_OK;
+ }
+
+ c->buffer->last += n;
+
+ rc = ph->handler(s);
+
+ if (rc != NGX_AGAIN) {
+ return rc;
+ }
+
+ if (c->buffer->last == c->buffer->end) {
+ ngx_log_error(NGX_LOG_ERR, c->log, 0, "preread buffer full");
+ return NGX_STREAM_BAD_REQUEST;
+ }
+ }
+
+ return NGX_AGAIN;
+}
+
+
ngx_int_t
ngx_stream_core_content_phase(ngx_stream_session_t *s,
ngx_stream_phase_handler_t *ph)
More information about the nginx-devel
mailing list