[PATCH 1 of 3] Stream: socket peek in preread phase

Sergey Kandaurov pluknet at nginx.com
Thu Jan 18 13:43:08 UTC 2024


> 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?

> +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


More information about the nginx-devel mailing list