quic: correct http3 callback and fix revealed errs · nodejs/node@2ef4b7e · GitHub
Skip to content

Commit 2ef4b7e

Browse files
martenrichteraduh95
martenrichter
authored and
aduh95
committed
quic: correct http3 callback and fix revealed errs
The http3 application had misinterpreted some of nghttp3 callbacks regarding stopSending and ResetStream. Actually, these callbacks asks the application to do the action and not informs about an event from the peer. The fixes lead to some failures of the automated tests, uncovering some problems: First headers, and pendingTrailers were reset, when the internal object went away, though the test wanted to read them. Second, during a graceful session shutdown, the implemented did not waited for all stream to be removed, but only one. Fixes: #63657 Signed-off-by: Marten Richter <marten.richter@freenet.de> PR-URL: #64289 Fixes: #63657 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent cbb2568 commit 2ef4b7e

5 files changed

Lines changed: 70 additions & 47 deletions

File tree

‎lib/internal/quic/quic.js‎

Lines changed: 2 additions & 2 deletions

‎src/quic/http3.cc‎

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -921,24 +921,25 @@ class Http3ApplicationImpl final : public Session::Application {
921921
stream->ReceiveData(nullptr, 0, flags);
922922
}
923923

924-
void OnStopSending(stream_id id, error_code app_error_code) {
924+
void OnSendStopSending(stream_id id, error_code app_error_code) {
925925
auto stream = session().FindStream(id);
926926
if (!stream) [[unlikely]]
927927
return;
928928
Debug(&session(),
929-
"HTTP/3 application received stop sending for stream %" PRIi64,
929+
"HTTP/3 application should send stop sending for stream %" PRIi64,
930930
id);
931-
stream->ReceiveStopSending(QuicError::ForApplication(app_error_code));
931+
stream->SendStopSending(app_error_code);
932932
}
933933

934-
void OnResetStream(stream_id id, error_code app_error_code) {
934+
void OnDoResetStream(stream_id id, error_code app_error_code) {
935935
auto stream = session().FindStream(id);
936936
if (!stream) [[unlikely]]
937937
return;
938938
Debug(&session(),
939-
"HTTP/3 application received reset stream for stream %" PRIi64,
939+
"HTTP/3 application received a request to reset stream for stream "
940+
"%" PRIi64,
940941
id);
941-
stream->ReceiveStreamReset(0, QuicError::ForApplication(app_error_code));
942+
stream->DoStreamReset(app_error_code);
942943
}
943944

944945
void OnShutdown(stream_id id) {
@@ -1318,29 +1319,31 @@ class Http3ApplicationImpl final : public Session::Application {
13181319
return NGTCP2_SUCCESS;
13191320
}
13201321

1321-
static int on_stop_sending(nghttp3_conn* conn,
1322-
stream_id id,
1323-
error_code app_error_code,
1324-
void* conn_user_data,
1325-
void* stream_user_data) {
1322+
static int on_send_stop_sending(nghttp3_conn* conn,
1323+
stream_id id,
1324+
error_code app_error_code,
1325+
void* conn_user_data,
1326+
void* stream_user_data) {
1327+
// this callback asks the app side to send a stop sending
13261328
NGHTTP3_CALLBACK_SCOPE(app);
13271329
if (app.is_control_stream(id)) [[unlikely]] {
13281330
return NGHTTP3_ERR_CALLBACK_FAILURE;
13291331
}
1330-
app.OnStopSending(id, app_error_code);
1332+
app.OnSendStopSending(id, app_error_code);
13311333
return NGTCP2_SUCCESS;
13321334
}
13331335

1334-
static int on_reset_stream(nghttp3_conn* conn,
1335-
stream_id id,
1336-
error_code app_error_code,
1337-
void* conn_user_data,
1338-
void* stream_user_data) {
1336+
static int on_do_reset_stream(nghttp3_conn* conn,
1337+
stream_id id,
1338+
error_code app_error_code,
1339+
void* conn_user_data,
1340+
void* stream_user_data) {
1341+
// this callback ask the app side to do a reset stream
13391342
NGHTTP3_CALLBACK_SCOPE(app);
13401343
if (app.is_control_stream(id)) [[unlikely]] {
13411344
return NGHTTP3_ERR_CALLBACK_FAILURE;
13421345
}
1343-
app.OnResetStream(id, app_error_code);
1346+
app.OnDoResetStream(id, app_error_code);
13441347
return NGTCP2_SUCCESS;
13451348
}
13461349

@@ -1394,9 +1397,9 @@ class Http3ApplicationImpl final : public Session::Application {
13941397
on_begin_trailers,
13951398
on_receive_trailer,
13961399
on_end_trailers,
1397-
on_stop_sending,
1400+
on_send_stop_sending,
13981401
on_end_stream,
1399-
on_reset_stream,
1402+
on_do_reset_stream,
14001403
on_shutdown,
14011404
nullptr, // recv_settings (deprecated)
14021405
on_receive_origin,

‎src/quic/session.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2806,7 +2806,8 @@ void Session::RemoveStream(stream_id id) {
28062806
// then we can proceed to finishing the close now. Note that the
28072807
// expectation is that the session will be destroyed once FinishClose
28082808
// returns.
2809-
if (impl_->state()->closing && impl_->state()->graceful_close) {
2809+
if (impl_->state()->closing && impl_->state()->graceful_close &&
2810+
impl_->streams_.size() == 0) {
28102811
FinishClose();
28112812
CHECK(is_destroyed());
28122813
}

‎src/quic/streams.cc‎

Lines changed: 32 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -487,15 +487,7 @@ struct Stream::Impl {
487487
code = args[0].As<BigInt>()->Uint64Value(&unused);
488488
}
489489

490-
stream->EndReadable();
491-
492-
if (!stream->is_pending()) {
493-
// If the stream is a local unidirectional there's nothing to do here.
494-
if (stream->is_local_unidirectional()) return;
495-
stream->NotifyReadableEnded(code);
496-
} else {
497-
stream->pending_close_read_code_ = code;
498-
}
490+
stream->SendStopSending(code);
499491
}
500492

501493
// Sends a reset stream to the peer to tell it we will not be sending any
@@ -512,21 +504,7 @@ struct Stream::Impl {
512504
code = args[0].As<BigInt>()->Uint64Value(&lossless);
513505
}
514506

515-
if (stream->state()->reset == 1) return;
516-
517-
stream->EndWritable();
518-
// We can release our outbound here now. Since the stream is being reset
519-
// on the ngtcp2 side, we do not need to keep any of the data around
520-
// waiting for acknowledgement that will never come.
521-
stream->outbound_.reset();
522-
stream->state()->reset = 1;
523-
524-
if (!stream->is_pending()) {
525-
if (stream->is_remote_unidirectional()) return;
526-
stream->NotifyWritableEnded(code);
527-
} else {
528-
stream->pending_close_write_code_ = code;
529-
}
507+
stream->DoStreamReset(code);
530508
}
531509

532510
JS_METHOD(SetPriority) {
@@ -1827,6 +1805,36 @@ void Stream::ReceiveStreamReset(uint64_t final_size, QuicError error) {
18271805
EmitReset(error);
18281806
}
18291807

1808+
void Stream::DoStreamReset(error_code code) {
1809+
if (state()->reset == 1) return;
1810+
1811+
EndWritable();
1812+
// We can release our outbound here now. Since the stream is being reset
1813+
// on the ngtcp2 side, we do not need to keep any of the data around
1814+
// waiting for acknowledgement that will never come.
1815+
outbound_.reset();
1816+
state()->reset = 1;
1817+
1818+
if (!is_pending()) {
1819+
if (is_remote_unidirectional()) return;
1820+
NotifyWritableEnded(code);
1821+
} else {
1822+
pending_close_write_code_ = code;
1823+
}
1824+
}
1825+
1826+
void Stream::SendStopSending(error_code code) {
1827+
EndReadable();
1828+
1829+
if (!is_pending()) {
1830+
// If the stream is a local unidirectional there's nothing to do here.
1831+
if (is_local_unidirectional()) return;
1832+
NotifyReadableEnded(code);
1833+
} else {
1834+
pending_close_read_code_ = code;
1835+
}
1836+
}
1837+
18301838
// ============================================================================
18311839

18321840
void Stream::EmitBlocked() {

‎src/quic/streams.h‎

Lines changed: 11 additions & 0 deletions

0 commit comments

Comments
 (0)