[PATCH] QUIC: defer setting the active flag for client stream events

[Roman Arutyunyan]

On Tue, Jan 17, 2023 at 04:01:39PM +0400, Sergey Kandaurov wrote:
> 
> > On 17 Jan 2023, at 14:44, Roman Arutyunyan <arut at nginx.com> wrote:
> > 
> > 
> > 
> >> On 16 Jan 2023, at 17:27, Sergey Kandaurov <pluknet at nginx.com> wrote:
> >> 
> >> # HG changeset patch
> >> # User Sergey Kandaurov <pluknet at nginx.com>
> >> # Date 1673875616 -14400
> >> #      Mon Jan 16 17:26:56 2023 +0400
> >> # Branch quic
> >> # Node ID f7c7cabe232898db5b16a142163de71964cebcfd
> >> # Parent  6bb884dc72916dc675df65d02abee0c9cfabc916
> >> QUIC: defer setting the active flag for client stream events.
> >> 
> >> Specifically, now it is kept unset until streams are initialized.
> >> Notably, this unbreaks OCSP with client certificates after 35e27117b593.
> >> Previously, the read event could be posted prematurely in ngx_quic_set_event()
> >> e.g., as part of handling a STREAM frame.
> >> 
> >> diff --git a/src/event/quic/ngx_event_quic_streams.c b/src/event/quic/ngx_event_quic_streams.c
> >> --- a/src/event/quic/ngx_event_quic_streams.c
> >> +++ b/src/event/quic/ngx_event_quic_streams.c
> >> @@ -106,6 +106,13 @@ ngx_quic_open_stream(ngx_connection_t *c
> >>         return NULL;
> >>     }
> >> 
> >> +    nqs->connection->write->active = 1;
> >> +    nqs->connection->write->ready = 1;
> >> +
> >> +    if (!bidi) {
> > 
> > if (bidi) ?
> 
> Sure, tnx.
> 
> > 
> >> +        nqs->connection->read->active = 1;
> >> +    }
> >> +
> >>     return nqs->connection;
> > 
> > This could've been simplified by adding a variable for nqs->connection, YMMV.
> 
> Agree, and it looks similar to other places.
> 
> While here, renamed nqs to just a qs, the only one after 6434160b4b78.
> 
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1673956193 -14400
> #      Tue Jan 17 15:49:53 2023 +0400
> # Branch quic
> # Node ID fa3018f0102247215953126515974c76d2b02e53
> # Parent  6bb884dc72916dc675df65d02abee0c9cfabc916
> QUIC: defer setting the active flag for client stream events.
> 
> Specifically, now it is kept unset until streams are initialized.
> Notably, this unbreaks OCSP with client certificates after 35e27117b593.
> Previously, the read event could be posted prematurely via ngx_quic_set_event()
> e.g., as part of handling a STREAM frame.
> 
> diff --git a/src/event/quic/ngx_event_quic_streams.c b/src/event/quic/ngx_event_quic_streams.c
> --- a/src/event/quic/ngx_event_quic_streams.c
> +++ b/src/event/quic/ngx_event_quic_streams.c
> @@ -46,8 +46,8 @@ ngx_connection_t *
>  ngx_quic_open_stream(ngx_connection_t *c, ngx_uint_t bidi)
>  {
>      uint64_t                id;
> -    ngx_connection_t       *pc;
> -    ngx_quic_stream_t      *nqs;
> +    ngx_connection_t       *pc, *sc;
> +    ngx_quic_stream_t      *qs;
>      ngx_quic_connection_t  *qc;
>  
>      pc = c->quic ? c->quic->parent : c;
> @@ -101,12 +101,21 @@ ngx_quic_open_stream(ngx_connection_t *c
>          qc->streams.server_streams_uni++;
>      }
>  
> -    nqs = ngx_quic_create_stream(pc, id);
> -    if (nqs == NULL) {
> +    qs = ngx_quic_create_stream(pc, id);
> +    if (qs == NULL) {
>          return NULL;
>      }
>  
> -    return nqs->connection;
> +    sc = qs->connection;
> +
> +    sc->write->active = 1;
> +    sc->write->ready = 1;
> +
> +    if (bidi) {
> +        sc->read->active = 1;
> +    }
> +
> +    return sc;
>  }
>  
>  
> @@ -534,6 +543,13 @@ ngx_quic_init_stream_handler(ngx_event_t
>  
>      ngx_log_debug0(NGX_LOG_DEBUG_EVENT, c->log, 0, "quic init stream");
>  
> +    if ((qs->id & NGX_QUIC_STREAM_UNIDIRECTIONAL) == 0) {
> +        c->write->active = 1;
> +        c->write->ready = 1;
> +    }
> +
> +    c->read->active = 1;
> +
>      ngx_queue_remove(&qs->queue);
>  
>      c->listening->handler(c);
> @@ -704,19 +720,6 @@ ngx_quic_create_stream(ngx_connection_t 
>  
>      log->connection = sc->number;
>  
> -    if ((id & NGX_QUIC_STREAM_UNIDIRECTIONAL) == 0
> -        || (id & NGX_QUIC_STREAM_SERVER_INITIATED))
> -    {
> -        sc->write->active = 1;
> -        sc->write->ready = 1;
> -    }
> -
> -    if ((id & NGX_QUIC_STREAM_UNIDIRECTIONAL) == 0
> -        || (id & NGX_QUIC_STREAM_SERVER_INITIATED) == 0)
> -    {
> -        sc->read->active = 1;
> -    }
> -
>      if (id & NGX_QUIC_STREAM_UNIDIRECTIONAL) {
>          if (id & NGX_QUIC_STREAM_SERVER_INITIATED) {
>              qs->send_max_data = qc->ctp.initial_max_stream_data_uni;
> 
> 
> -- 
> Sergey Kandaurov
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

Looks fine

--
Roman Arutyunyan