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

[Sergey Kandaurov]

> 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