Commit 2ef4b7e

Browse files
martenrichteraduh95
authored andcommitted
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: #64289Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -2543,8 +2543,8 @@ class QuicStream {
25432543
inner.ontrailers=undefined;
25442544
inner.oninfo=undefined;
25452545
inner.onwanttrailers=undefined;
2546-
inner.headers=undefined;
2547-
inner.pendingTrailers=undefined;
2546+
// Do not reset headers here, this is still important information
2547+
// the same applies for pendingTrailers
25482548
this.#handle =undefined;
25492549
if(inner.fileHandle!==undefined){
25502550
// Close the FileHandle that was used as a body source. The close

‎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-
voidOnStopSending(stream_id id, error_code app_error_code) {
924+
voidOnSendStopSending(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-
voidOnResetStream(stream_id id, error_code app_error_code) {
934+
voidOnDoResetStream(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
voidOnShutdown(stream_id id) {
@@ -1318,29 +1319,31 @@ class Http3ApplicationImpl final : public Session::Application {
13181319
returnNGTCP2_SUCCESS;
13191320
}
13201321

1321-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13291331
}
1330-
app.OnStopSending(id, app_error_code);
1332+
app.OnSendStopSending(id, app_error_code);
13311333
returnNGTCP2_SUCCESS;
13321334
}
13331335

1334-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13421345
}
1343-
app.OnResetStream(id, app_error_code);
1346+
app.OnDoResetStream(id, app_error_code);
13441347
returnNGTCP2_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+
voidStream::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+
voidStream::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
voidStream::EmitBlocked() {

‎src/quic/streams.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,17 @@ class Stream final : public AsyncWrap,
344344
voidReceiveStopSending(QuicError error);
345345
voidReceiveStreamReset(uint64_t final_size, QuicError error);
346346

347+
// Sends a reset stream to the peer to tell it we will not be sending any
348+
// more data for this stream. This has the effect of shutting down the
349+
// writable side of the stream for this peer. Any data that is held in the
350+
// outbound queue will be dropped. The stream may still be readable.
351+
voidDoStreamReset(error_code code);
352+
353+
// Tells the peer to stop sending data for this stream. This has the effect
354+
// of shutting down the readable side of the stream for this peer. Any data
355+
// that has already been received is still readable.
356+
voidSendStopSending(error_code code);
357+
347358
// Currently, only HTTP/3 streams support headers. These methods are here
348359
// to support that. They are not used when using any other QUIC application.
349360

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Commit 2ef4b7e

Browse files
martenrichteraduh95
authored andcommitted
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: #64289Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -2543,8 +2543,8 @@ class QuicStream {
25432543
inner.ontrailers=undefined;
25442544
inner.oninfo=undefined;
25452545
inner.onwanttrailers=undefined;
2546-
inner.headers=undefined;
2547-
inner.pendingTrailers=undefined;
2546+
// Do not reset headers here, this is still important information
2547+
// the same applies for pendingTrailers
25482548
this.#handle =undefined;
25492549
if(inner.fileHandle!==undefined){
25502550
// Close the FileHandle that was used as a body source. The close

‎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-
voidOnStopSending(stream_id id, error_code app_error_code) {
924+
voidOnSendStopSending(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-
voidOnResetStream(stream_id id, error_code app_error_code) {
934+
voidOnDoResetStream(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
voidOnShutdown(stream_id id) {
@@ -1318,29 +1319,31 @@ class Http3ApplicationImpl final : public Session::Application {
13181319
returnNGTCP2_SUCCESS;
13191320
}
13201321

1321-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13291331
}
1330-
app.OnStopSending(id, app_error_code);
1332+
app.OnSendStopSending(id, app_error_code);
13311333
returnNGTCP2_SUCCESS;
13321334
}
13331335

1334-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13421345
}
1343-
app.OnResetStream(id, app_error_code);
1346+
app.OnDoResetStream(id, app_error_code);
13441347
returnNGTCP2_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+
voidStream::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+
voidStream::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
voidStream::EmitBlocked() {

‎src/quic/streams.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,17 @@ class Stream final : public AsyncWrap,
344344
voidReceiveStopSending(QuicError error);
345345
voidReceiveStreamReset(uint64_t final_size, QuicError error);
346346

347+
// Sends a reset stream to the peer to tell it we will not be sending any
348+
// more data for this stream. This has the effect of shutting down the
349+
// writable side of the stream for this peer. Any data that is held in the
350+
// outbound queue will be dropped. The stream may still be readable.
351+
voidDoStreamReset(error_code code);
352+
353+
// Tells the peer to stop sending data for this stream. This has the effect
354+
// of shutting down the readable side of the stream for this peer. Any data
355+
// that has already been received is still readable.
356+
voidSendStopSending(error_code code);
357+
347358
// Currently, only HTTP/3 streams support headers. These methods are here
348359
// to support that. They are not used when using any other QUIC application.
349360

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit 2ef4b7e

Browse files
martenrichteraduh95
authored andcommitted
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: #64289Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -2543,8 +2543,8 @@ class QuicStream {
25432543
inner.ontrailers=undefined;
25442544
inner.oninfo=undefined;
25452545
inner.onwanttrailers=undefined;
2546-
inner.headers=undefined;
2547-
inner.pendingTrailers=undefined;
2546+
// Do not reset headers here, this is still important information
2547+
// the same applies for pendingTrailers
25482548
this.#handle =undefined;
25492549
if(inner.fileHandle!==undefined){
25502550
// Close the FileHandle that was used as a body source. The close

‎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-
voidOnStopSending(stream_id id, error_code app_error_code) {
924+
voidOnSendStopSending(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-
voidOnResetStream(stream_id id, error_code app_error_code) {
934+
voidOnDoResetStream(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
voidOnShutdown(stream_id id) {
@@ -1318,29 +1319,31 @@ class Http3ApplicationImpl final : public Session::Application {
13181319
returnNGTCP2_SUCCESS;
13191320
}
13201321

1321-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13291331
}
1330-
app.OnStopSending(id, app_error_code);
1332+
app.OnSendStopSending(id, app_error_code);
13311333
returnNGTCP2_SUCCESS;
13321334
}
13331335

1334-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13421345
}
1343-
app.OnResetStream(id, app_error_code);
1346+
app.OnDoResetStream(id, app_error_code);
13441347
returnNGTCP2_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+
voidStream::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+
voidStream::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
voidStream::EmitBlocked() {

‎src/quic/streams.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,17 @@ class Stream final : public AsyncWrap,
344344
voidReceiveStopSending(QuicError error);
345345
voidReceiveStreamReset(uint64_t final_size, QuicError error);
346346

347+
// Sends a reset stream to the peer to tell it we will not be sending any
348+
// more data for this stream. This has the effect of shutting down the
349+
// writable side of the stream for this peer. Any data that is held in the
350+
// outbound queue will be dropped. The stream may still be readable.
351+
voidDoStreamReset(error_code code);
352+
353+
// Tells the peer to stop sending data for this stream. This has the effect
354+
// of shutting down the readable side of the stream for this peer. Any data
355+
// that has already been received is still readable.
356+
voidSendStopSending(error_code code);
357+
347358
// Currently, only HTTP/3 streams support headers. These methods are here
348359
// to support that. They are not used when using any other QUIC application.
349360

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit 2ef4b7e

Browse files
martenrichteraduh95
authored andcommitted
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: #64289Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -2543,8 +2543,8 @@ class QuicStream {
25432543
inner.ontrailers=undefined;
25442544
inner.oninfo=undefined;
25452545
inner.onwanttrailers=undefined;
2546-
inner.headers=undefined;
2547-
inner.pendingTrailers=undefined;
2546+
// Do not reset headers here, this is still important information
2547+
// the same applies for pendingTrailers
25482548
this.#handle =undefined;
25492549
if(inner.fileHandle!==undefined){
25502550
// Close the FileHandle that was used as a body source. The close

‎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-
voidOnStopSending(stream_id id, error_code app_error_code) {
924+
voidOnSendStopSending(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-
voidOnResetStream(stream_id id, error_code app_error_code) {
934+
voidOnDoResetStream(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
voidOnShutdown(stream_id id) {
@@ -1318,29 +1319,31 @@ class Http3ApplicationImpl final : public Session::Application {
13181319
returnNGTCP2_SUCCESS;
13191320
}
13201321

1321-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13291331
}
1330-
app.OnStopSending(id, app_error_code);
1332+
app.OnSendStopSending(id, app_error_code);
13311333
returnNGTCP2_SUCCESS;
13321334
}
13331335

1334-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13421345
}
1343-
app.OnResetStream(id, app_error_code);
1346+
app.OnDoResetStream(id, app_error_code);
13441347
returnNGTCP2_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+
voidStream::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+
voidStream::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
voidStream::EmitBlocked() {

‎src/quic/streams.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,17 @@ class Stream final : public AsyncWrap,
344344
voidReceiveStopSending(QuicError error);
345345
voidReceiveStreamReset(uint64_t final_size, QuicError error);
346346

347+
// Sends a reset stream to the peer to tell it we will not be sending any
348+
// more data for this stream. This has the effect of shutting down the
349+
// writable side of the stream for this peer. Any data that is held in the
350+
// outbound queue will be dropped. The stream may still be readable.
351+
voidDoStreamReset(error_code code);
352+
353+
// Tells the peer to stop sending data for this stream. This has the effect
354+
// of shutting down the readable side of the stream for this peer. Any data
355+
// that has already been received is still readable.
356+
voidSendStopSending(error_code code);
357+
347358
// Currently, only HTTP/3 streams support headers. These methods are here
348359
// to support that. They are not used when using any other QUIC application.
349360

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Commit 2ef4b7e

Browse files
martenrichteraduh95
authored andcommitted
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: #64289Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -2543,8 +2543,8 @@ class QuicStream {
25432543
inner.ontrailers=undefined;
25442544
inner.oninfo=undefined;
25452545
inner.onwanttrailers=undefined;
2546-
inner.headers=undefined;
2547-
inner.pendingTrailers=undefined;
2546+
// Do not reset headers here, this is still important information
2547+
// the same applies for pendingTrailers
25482548
this.#handle =undefined;
25492549
if(inner.fileHandle!==undefined){
25502550
// Close the FileHandle that was used as a body source. The close

‎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-
voidOnStopSending(stream_id id, error_code app_error_code) {
924+
voidOnSendStopSending(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-
voidOnResetStream(stream_id id, error_code app_error_code) {
934+
voidOnDoResetStream(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
voidOnShutdown(stream_id id) {
@@ -1318,29 +1319,31 @@ class Http3ApplicationImpl final : public Session::Application {
13181319
returnNGTCP2_SUCCESS;
13191320
}
13201321

1321-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13291331
}
1330-
app.OnStopSending(id, app_error_code);
1332+
app.OnSendStopSending(id, app_error_code);
13311333
returnNGTCP2_SUCCESS;
13321334
}
13331335

1334-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13421345
}
1343-
app.OnResetStream(id, app_error_code);
1346+
app.OnDoResetStream(id, app_error_code);
13441347
returnNGTCP2_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+
voidStream::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+
voidStream::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
voidStream::EmitBlocked() {

‎src/quic/streams.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,17 @@ class Stream final : public AsyncWrap,
344344
voidReceiveStopSending(QuicError error);
345345
voidReceiveStreamReset(uint64_t final_size, QuicError error);
346346

347+
// Sends a reset stream to the peer to tell it we will not be sending any
348+
// more data for this stream. This has the effect of shutting down the
349+
// writable side of the stream for this peer. Any data that is held in the
350+
// outbound queue will be dropped. The stream may still be readable.
351+
voidDoStreamReset(error_code code);
352+
353+
// Tells the peer to stop sending data for this stream. This has the effect
354+
// of shutting down the readable side of the stream for this peer. Any data
355+
// that has already been received is still readable.
356+
voidSendStopSending(error_code code);
357+
347358
// Currently, only HTTP/3 streams support headers. These methods are here
348359
// to support that. They are not used when using any other QUIC application.
349360

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit 2ef4b7e

Browse files
martenrichteraduh95
authored andcommitted
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: #64289Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -2543,8 +2543,8 @@ class QuicStream {
25432543
inner.ontrailers=undefined;
25442544
inner.oninfo=undefined;
25452545
inner.onwanttrailers=undefined;
2546-
inner.headers=undefined;
2547-
inner.pendingTrailers=undefined;
2546+
// Do not reset headers here, this is still important information
2547+
// the same applies for pendingTrailers
25482548
this.#handle =undefined;
25492549
if(inner.fileHandle!==undefined){
25502550
// Close the FileHandle that was used as a body source. The close

‎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-
voidOnStopSending(stream_id id, error_code app_error_code) {
924+
voidOnSendStopSending(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-
voidOnResetStream(stream_id id, error_code app_error_code) {
934+
voidOnDoResetStream(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
voidOnShutdown(stream_id id) {
@@ -1318,29 +1319,31 @@ class Http3ApplicationImpl final : public Session::Application {
13181319
returnNGTCP2_SUCCESS;
13191320
}
13201321

1321-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13291331
}
1330-
app.OnStopSending(id, app_error_code);
1332+
app.OnSendStopSending(id, app_error_code);
13311333
returnNGTCP2_SUCCESS;
13321334
}
13331335

1334-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13421345
}
1343-
app.OnResetStream(id, app_error_code);
1346+
app.OnDoResetStream(id, app_error_code);
13441347
returnNGTCP2_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+
voidStream::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+
voidStream::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
voidStream::EmitBlocked() {

‎src/quic/streams.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,17 @@ class Stream final : public AsyncWrap,
344344
voidReceiveStopSending(QuicError error);
345345
voidReceiveStreamReset(uint64_t final_size, QuicError error);
346346

347+
// Sends a reset stream to the peer to tell it we will not be sending any
348+
// more data for this stream. This has the effect of shutting down the
349+
// writable side of the stream for this peer. Any data that is held in the
350+
// outbound queue will be dropped. The stream may still be readable.
351+
voidDoStreamReset(error_code code);
352+
353+
// Tells the peer to stop sending data for this stream. This has the effect
354+
// of shutting down the readable side of the stream for this peer. Any data
355+
// that has already been received is still readable.
356+
voidSendStopSending(error_code code);
357+
347358
// Currently, only HTTP/3 streams support headers. These methods are here
348359
// to support that. They are not used when using any other QUIC application.
349360

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit 2ef4b7e

Browse files
martenrichteraduh95
authored andcommitted
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: #64289Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -2543,8 +2543,8 @@ class QuicStream {
25432543
inner.ontrailers=undefined;
25442544
inner.oninfo=undefined;
25452545
inner.onwanttrailers=undefined;
2546-
inner.headers=undefined;
2547-
inner.pendingTrailers=undefined;
2546+
// Do not reset headers here, this is still important information
2547+
// the same applies for pendingTrailers
25482548
this.#handle =undefined;
25492549
if(inner.fileHandle!==undefined){
25502550
// Close the FileHandle that was used as a body source. The close

‎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-
voidOnStopSending(stream_id id, error_code app_error_code) {
924+
voidOnSendStopSending(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-
voidOnResetStream(stream_id id, error_code app_error_code) {
934+
voidOnDoResetStream(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
voidOnShutdown(stream_id id) {
@@ -1318,29 +1319,31 @@ class Http3ApplicationImpl final : public Session::Application {
13181319
returnNGTCP2_SUCCESS;
13191320
}
13201321

1321-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13291331
}
1330-
app.OnStopSending(id, app_error_code);
1332+
app.OnSendStopSending(id, app_error_code);
13311333
returnNGTCP2_SUCCESS;
13321334
}
13331335

1334-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13421345
}
1343-
app.OnResetStream(id, app_error_code);
1346+
app.OnDoResetStream(id, app_error_code);
13441347
returnNGTCP2_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+
voidStream::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+
voidStream::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
voidStream::EmitBlocked() {

‎src/quic/streams.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,17 @@ class Stream final : public AsyncWrap,
344344
voidReceiveStopSending(QuicError error);
345345
voidReceiveStreamReset(uint64_t final_size, QuicError error);
346346

347+
// Sends a reset stream to the peer to tell it we will not be sending any
348+
// more data for this stream. This has the effect of shutting down the
349+
// writable side of the stream for this peer. Any data that is held in the
350+
// outbound queue will be dropped. The stream may still be readable.
351+
voidDoStreamReset(error_code code);
352+
353+
// Tells the peer to stop sending data for this stream. This has the effect
354+
// of shutting down the readable side of the stream for this peer. Any data
355+
// that has already been received is still readable.
356+
voidSendStopSending(error_code code);
357+
347358
// Currently, only HTTP/3 streams support headers. These methods are here
348359
// to support that. They are not used when using any other QUIC application.
349360

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Commit 2ef4b7e

Browse files
martenrichteraduh95
authored andcommitted
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: #64289Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -2543,8 +2543,8 @@ class QuicStream {
25432543
inner.ontrailers=undefined;
25442544
inner.oninfo=undefined;
25452545
inner.onwanttrailers=undefined;
2546-
inner.headers=undefined;
2547-
inner.pendingTrailers=undefined;
2546+
// Do not reset headers here, this is still important information
2547+
// the same applies for pendingTrailers
25482548
this.#handle =undefined;
25492549
if(inner.fileHandle!==undefined){
25502550
// Close the FileHandle that was used as a body source. The close

‎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-
voidOnStopSending(stream_id id, error_code app_error_code) {
924+
voidOnSendStopSending(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-
voidOnResetStream(stream_id id, error_code app_error_code) {
934+
voidOnDoResetStream(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
voidOnShutdown(stream_id id) {
@@ -1318,29 +1319,31 @@ class Http3ApplicationImpl final : public Session::Application {
13181319
returnNGTCP2_SUCCESS;
13191320
}
13201321

1321-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13291331
}
1330-
app.OnStopSending(id, app_error_code);
1332+
app.OnSendStopSending(id, app_error_code);
13311333
returnNGTCP2_SUCCESS;
13321334
}
13331335

1334-
staticinton_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+
staticinton_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
returnNGHTTP3_ERR_CALLBACK_FAILURE;
13421345
}
1343-
app.OnResetStream(id, app_error_code);
1346+
app.OnDoResetStream(id, app_error_code);
13441347
returnNGTCP2_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+
voidStream::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+
voidStream::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
voidStream::EmitBlocked() {

‎src/quic/streams.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,17 @@ class Stream final : public AsyncWrap,
344344
voidReceiveStopSending(QuicError error);
345345
voidReceiveStreamReset(uint64_t final_size, QuicError error);
346346

347+
// Sends a reset stream to the peer to tell it we will not be sending any
348+
// more data for this stream. This has the effect of shutting down the
349+
// writable side of the stream for this peer. Any data that is held in the
350+
// outbound queue will be dropped. The stream may still be readable.
351+
voidDoStreamReset(error_code code);
352+
353+
// Tells the peer to stop sending data for this stream. This has the effect
354+
// of shutting down the readable side of the stream for this peer. Any data
355+
// that has already been received is still readable.
356+
voidSendStopSending(error_code code);
357+
347358
// Currently, only HTTP/3 streams support headers. These methods are here
348359
// to support that. They are not used when using any other QUIC application.
349360

0 commit comments

Comments
 (0)