Hello!

On Fri, Jan 19, 2024 at 03:42:37PM +0400, Sergey Kandaurov wrote:

>
> > On 18 Jan 2024, at 23:44, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Thu, Jan 18, 2024 at 08:15:33PM +0400, Sergey Kandaurov wrote:
> >
> >> On Thu, Jan 18, 2024 at 07:06:06PM +0400, Roman Arutyunyan wrote:
> >>> Hi,
> >>>
> >>> # HG changeset patch
> >>> # User Roman Arutyunyan <arut@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)
> >>
> >> Looks good.
> >
> > I'm somewhat sceptical about the idea of functionality which
> > depends on the edge-triggered event methods being available.
> >
>
> We discussed this with Roman days back:
> https://mailman.nginx.org/pipermail/nginx-devel/2023-December/O6XC56D5BPSE6F45IZSRYEKOSGL3VVYB.html

Yes, I've seen the discussion. I'm not convinced that proposed
approach is a good solution though, and would like to re-visit
this after the initial review.

> > If/when the patch series is reviewed, please make sure it gets my
> > approval before commit.
> >
>
> You're welcome to participate.

Sure.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel