GeezerVison is showing github.com — quic: fix stop sending behaviour & callback · nodejs/node@8a00872
File treeExpand file treeCollapse file tree Open diff view settings Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: doc/api/quic.md +21-8Lines changed: 21 additions & 8 deletions - Display the source diff
- Display the rich diff
| Original file line number | Diff line number | Diff line change |
|---|
@@ -2013,8 +2013,7 @@ added: v23.8.0 | 2013 | 2013 |
| 2014 | 2014 | The callback to invoke when the peer aborts a direction of the stream by | 2015 | 2015 | sending a `RESET_STREAM` frame (the peer abandons their writable side, so | 2016 | | -no further data will arrive on our readable side) or a `STOP_SENDING` | 2017 | | -frame (the peer asks us to stop writing on our writable side). | | 2016 | +no further data will arrive on our readable side). | 2018 | 2017 |
| 2019 | 2018 | The callback receives a Node.js error whose `errorCode` (`bigint`) | 2020 | 2019 | property carries the application error code from the wire frame. | @@ -2025,6 +2024,21 @@ continue using the still-active direction on a bidirectional stream), | 2025 | 2024 | abort the other direction with [`writer.fail()`][], or tear down the | 2026 | 2025 | whole stream with [`stream.destroy()`][]. Read/write. | 2027 | 2026 |
| | 2027 | +### `stream.onstopsending` | | 2028 | + | | 2029 | +<!-- YAML | | 2030 | +added: REPLACEME | | 2031 | +--> | | 2032 | + | | 2033 | +* Type: {quic.OnStreamErrorCallback} | | 2034 | + | | 2035 | +The callback to invoke when the peer aborts a direction of the stream by | | 2036 | +sending a `STOP_SENDING` frame (the peer asks us to stop writing on our | | 2037 | +writable side). | | 2038 | + | | 2039 | +The callback receives a Node.js error whose `errorCode` (`bigint`) | | 2040 | +property carries the application error code from the wire frame. Read/write. | | 2041 | + | 2028 | 2042 | ### `stream.headers` | 2029 | 2043 |
| 2030 | 2044 | <!-- YAML | @@ -3569,8 +3583,8 @@ functions. If a callback throws synchronously or returns a promise that | 3569 | 3583 | rejects, the error is caught and the owning session or stream is destroyed | 3570 | 3584 | with that error: | 3571 | 3585 |
| 3572 | | -* Stream callbacks (`onblocked`, `onreset`, `onheaders`, `ontrailers`, | 3573 | | - `oninfo`, `onwanttrailers`): the stream is destroyed. | | 3586 | +* Stream callbacks (`onblocked`, `onreset`, `onstopsending`, `onheaders`, | | 3587 | + `ontrailers`, `oninfo`, `onwanttrailers`): the stream is destroyed. | 3574 | 3588 | * Session callbacks (`onapplication`, `onstream`, `ondatagram`, | 3575 | 3589 | `ondatagramstatus`, `onpathvalidation`, `onsessionticket`, | 3576 | 3590 | `onnewtoken`, `onversionnegotiation`, `onorigin`, `ongoaway`, | @@ -4439,10 +4453,9 @@ added: REPLACEME | 4439 | 4453 | * `session` {quic.QuicSession} | 4440 | 4454 | * `error` {any} The QUIC error associated with the reset. | 4441 | 4455 |
| 4442 | | -Published when a stream receives a STOP\_SENDING or RESET\_STREAM frame | 4443 | | -from the peer, indicating the peer has aborted the stream. This is a | 4444 | | -key signal for diagnosing application-level issues such as cancelled | 4445 | | -requests. | | 4456 | +Published when a stream receives a RESET\_STREAM frame from the peer, | | 4457 | +indicating the peer has aborted its sending direction. This is a key signal | | 4458 | +for diagnosing application-level issues such as cancelled requests. | 4446 | 4459 |
| 4447 | 4460 | ### Channel: `quic.stream.blocked` | 4448 | 4461 |
| |
Collapse file Copy file name to clipboardExpand all lines: lib/internal/quic/quic.js +50Lines changed: 50 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -12,6 +12,7 @@ const { | 12 | 12 | DataViewPrototypeGetByteLength, | 13 | 13 | ErrorCaptureStackTrace, | 14 | 14 | FunctionPrototypeBind, | | 15 | + FunctionPrototypeCall, | 15 | 16 | Number, | 16 | 17 | ObjectDefineProperties, | 17 | 18 | ObjectKeys, | @@ -210,6 +211,7 @@ const { | 210 | 211 | kSendHeaders, | 211 | 212 | kSessionApplication, | 212 | 213 | kSessionTicket, | | 214 | + kStopSending, | 213 | 215 | kTrailers, | 214 | 216 | kVersionNegotiation, | 215 | 217 | kInspect, | @@ -991,6 +993,14 @@ setCallbacks({ | 991 | 993 | this[kOwner][kReset](error); | 992 | 994 | }, | 993 | 995 |
| | 996 | + onStreamStopSending(error) { | | 997 | + if (error !== undefined) { | | 998 | + error = convertQuicError(error); | | 999 | + } | | 1000 | + debug('stream stop sending callback', this[kOwner], error); | | 1001 | + this[kOwner][kStopSending](error); | | 1002 | + }, | | 1003 | + | 994 | 1004 | onStreamHeaders(headers, kind) { | 995 | 1005 | // Called when the stream C++ handle has received a full block of headers. | 996 | 1006 | debug(`stream ${this[kOwner].id} headers callback`, headers, kind); | @@ -1576,6 +1586,7 @@ class QuicStream { | 1576 | 1586 | onerror: undefined, | 1577 | 1587 | onblocked: undefined, | 1578 | 1588 | onreset: undefined, | | 1589 | + onstopsending: undefined, | 1579 | 1590 | onheaders: undefined, | 1580 | 1591 | ontrailers: undefined, | 1581 | 1592 | oninfo: undefined, | @@ -1781,6 +1792,25 @@ class QuicStream { | 1781 | 1792 | } | 1782 | 1793 | } | 1783 | 1794 |
| | 1795 | + /** @type {OnStreamErrorCallback} */ | | 1796 | + get onstopsending() { | | 1797 | + assertIsQuicStream(this); | | 1798 | + return this.#inner.onstopsending; | | 1799 | + } | | 1800 | + | | 1801 | + set onstopsending(fn) { | | 1802 | + assertIsQuicStream(this); | | 1803 | + const inner = this.#inner; | | 1804 | + if (fn === undefined) { | | 1805 | + inner.onstopsending = undefined; | | 1806 | + inner.state.wantsStopSending = false; | | 1807 | + } else { | | 1808 | + validateFunction(fn, 'onstopsending'); | | 1809 | + inner.onstopsending = FunctionPrototypeBind(fn, this); | | 1810 | + inner.state.wantsStopSending = true; | | 1811 | + } | | 1812 | + } | | 1813 | + | 1784 | 1814 | /** @type {OnHeadersCallback} */ | 1785 | 1815 | get onheaders() { | 1786 | 1816 | assertIsQuicStream(this); | @@ -2143,6 +2173,19 @@ class QuicStream { | 2143 | 2173 | } | 2144 | 2174 | }; | 2145 | 2175 |
| | 2176 | + const onStopSending = stream[kStopSending]; | | 2177 | + stream[kStopSending] = (reason) => { | | 2178 | + if (!closed && !errored) { | | 2179 | + errored = true; | | 2180 | + error = reason; | | 2181 | + if (drainWakeup != null) { | | 2182 | + drainWakeup.reject(error); | | 2183 | + drainWakeup = null; | | 2184 | + } | | 2185 | + } | | 2186 | + FunctionPrototypeCall(onStopSending, stream, reason); | | 2187 | + }; | | 2188 | + | 2146 | 2189 | // A note on backpressure handling: per the stream/iter spec, the default | 2147 | 2190 | // backpressure policy for writers is strict, meaning that if the stream | 2148 | 2191 | // signals backpressure additional writes are rejected until the buffer has | @@ -2543,6 +2586,7 @@ class QuicStream { | 2543 | 2586 | inner.pendingClose.resolve = undefined; | 2544 | 2587 | inner.onblocked = undefined; | 2545 | 2588 | inner.onreset = undefined; | | 2589 | + inner.onstopsending = undefined; | 2546 | 2590 | inner.onheaders = undefined; | 2547 | 2591 | inner.onerror = undefined; | 2548 | 2592 | inner.ontrailers = undefined; | @@ -2596,6 +2640,12 @@ class QuicStream { | 2596 | 2640 | safeCallbackInvoke(inner.onreset, this, error); | 2597 | 2641 | } | 2598 | 2642 |
| | 2643 | + [kStopSending](error) { | | 2644 | + const inner = this.#inner; | | 2645 | + assert(inner.onstopsending, 'Unexpected stop sending event'); | | 2646 | + safeCallbackInvoke(inner.onstopsending, this, error); | | 2647 | + } | | 2648 | + | 2599 | 2649 | [kHeaders](headers, kind) { | 2600 | 2650 | const block = parseHeaderPairs(headers); | 2601 | 2651 | const kindName = kHeadersKindName[kind] ?? kind; | |
Collapse file Copy file name to clipboardExpand all lines: lib/internal/quic/state.js +24Lines changed: 24 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -101,6 +101,7 @@ const { | 101 | 101 | IDX_STATE_STREAM_WANTS_BLOCK, | 102 | 102 | IDX_STATE_STREAM_WANTS_HEADERS, | 103 | 103 | IDX_STATE_STREAM_WANTS_RESET, | | 104 | + IDX_STATE_STREAM_WANTS_STOP_SENDING, | 104 | 105 | IDX_STATE_STREAM_WANTS_TRAILERS, | 105 | 106 | IDX_STATE_STREAM_RECEIVED_EARLY_DATA, | 106 | 107 | IDX_STATE_STREAM_WRITE_DESIRED_SIZE, | @@ -142,6 +143,7 @@ assert(IDX_STATE_STREAM_HAS_READER !== undefined); | 142 | 143 | assert(IDX_STATE_STREAM_WANTS_BLOCK !== undefined); | 143 | 144 | assert(IDX_STATE_STREAM_WANTS_HEADERS !== undefined); | 144 | 145 | assert(IDX_STATE_STREAM_WANTS_RESET !== undefined); | | 146 | +assert(IDX_STATE_STREAM_WANTS_STOP_SENDING !== undefined); | 145 | 147 | assert(IDX_STATE_STREAM_WANTS_TRAILERS !== undefined); | 146 | 148 | assert(IDX_STATE_STREAM_WRITE_DESIRED_SIZE !== undefined); | 147 | 149 | assert(IDX_STATE_STREAM_RESET_CODE !== undefined); | @@ -826,6 +828,24 @@ class QuicStreamState { | 826 | 828 | DataViewPrototypeSetUint8(handle, this.#offset + IDX_STATE_STREAM_WANTS_RESET, val ? 1 : 0); | 827 | 829 | } | 828 | 830 |
| | 831 | + /** @type {boolean} */ | | 832 | + get wantsStopSending() { | | 833 | + const handle = this.#handle; | | 834 | + if (handle === undefined) return undefined; | | 835 | + return DataViewPrototypeGetUint8( | | 836 | + handle, this.#offset + IDX_STATE_STREAM_WANTS_STOP_SENDING) !== 0; | | 837 | + } | | 838 | + | | 839 | + /** @type {boolean} */ | | 840 | + set wantsStopSending(val) { | | 841 | + const handle = this.#handle; | | 842 | + if (handle === undefined) return; | | 843 | + DataViewPrototypeSetUint8( | | 844 | + handle, | | 845 | + this.#offset + IDX_STATE_STREAM_WANTS_STOP_SENDING, | | 846 | + val ? 1 : 0); | | 847 | + } | | 848 | + | 829 | 849 | /** @type {boolean} */ | 830 | 850 | get wantsTrailers() { | 831 | 851 | const handle = this.#handle; | @@ -903,6 +923,7 @@ class QuicStreamState { | 903 | 923 | hasReader, | 904 | 924 | wantsBlock, | 905 | 925 | wantsReset, | | 926 | + wantsStopSending, | 906 | 927 | wantsHeaders, | 907 | 928 | wantsTrailers, | 908 | 929 | early, | @@ -923,6 +944,7 @@ class QuicStreamState { | 923 | 944 | hasReader, | 924 | 945 | wantsBlock, | 925 | 946 | wantsReset, | | 947 | + wantsStopSending, | 926 | 948 | wantsHeaders, | 927 | 949 | wantsTrailers, | 928 | 950 | early, | @@ -960,6 +982,7 @@ class QuicStreamState { | 960 | 982 | hasReader, | 961 | 983 | wantsBlock, | 962 | 984 | wantsReset, | | 985 | + wantsStopSending, | 963 | 986 | wantsHeaders, | 964 | 987 | wantsTrailers, | 965 | 988 | early, | @@ -980,6 +1003,7 @@ class QuicStreamState { | 980 | 1003 | hasReader, | 981 | 1004 | wantsBlock, | 982 | 1005 | wantsReset, | | 1006 | + wantsStopSending, | 983 | 1007 | wantsHeaders, | 984 | 1008 | wantsTrailers, | 985 | 1009 | early, | |
Collapse file Copy file name to clipboardExpand all lines: lib/internal/quic/symbols.js +2Lines changed: 2 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -57,6 +57,7 @@ const kReset = Symbol('kReset'); | 57 | 57 | const kSendHeaders = Symbol('kSendHeaders'); | 58 | 58 | const kSessionApplication = Symbol('kSessionApplication'); | 59 | 59 | const kSessionTicket = Symbol('kSessionTicket'); | | 60 | +const kStopSending = Symbol('kStopSending'); | 60 | 61 | const kTrailers = Symbol('kTrailers'); | 61 | 62 | const kVersionNegotiation = Symbol('kVersionNegotiation'); | 62 | 63 |
| @@ -93,6 +94,7 @@ module.exports = { | 93 | 94 | kSendHeaders, | 94 | 95 | kSessionApplication, | 95 | 96 | kSessionTicket, | | 97 | + kStopSending, | 96 | 98 | kTrailers, | 97 | 99 | kVersionNegotiation, | 98 | 100 | }; | |
Collapse file Copy file name to clipboardExpand all lines: src/quic/bindingdata.h +1Lines changed: 1 addition & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -61,6 +61,7 @@ class SessionManager; | 61 | 61 | V(stream_drain, StreamDrain) \ | 62 | 62 | V(stream_headers, StreamHeaders) \ | 63 | 63 | V(stream_reset, StreamReset) \ | | 64 | + V(stream_stop_sending, StreamStopSending) \ | 64 | 65 | V(stream_trailers, StreamTrailers) | 65 | 66 |
| 66 | 67 | // The various JS strings the implementation uses. | |
Collapse file Copy file name to clipboardExpand all lines: src/quic/session.cc +11-11Lines changed: 11 additions & 11 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -1611,11 +1611,11 @@ struct Session::Impl final : public MemoryRetainer { | 1611 | 1611 | return NGTCP2_SUCCESS; | 1612 | 1612 | } | 1613 | 1613 |
| 1614 | | - static int on_stream_stop_sending(ngtcp2_conn* conn, | 1615 | | - stream_id stream_id, | 1616 | | - error_code app_error_code, | 1617 | | - void* user_data, | 1618 | | - void* stream_user_data) { | | 1614 | + static int on_receive_stream_stop_sending(ngtcp2_conn* conn, | | 1615 | + stream_id stream_id, | | 1616 | + error_code app_error_code, | | 1617 | + void* user_data, | | 1618 | + void* stream_user_data) { | 1619 | 1619 | NGTCP2_CALLBACK_SCOPE(session) | 1620 | 1620 | auto* stream = Stream::From(stream_user_data); | 1621 | 1621 | if (stream == nullptr) return NGTCP2_SUCCESS; | @@ -1652,7 +1652,7 @@ struct Session::Impl final : public MemoryRetainer { | 1652 | 1652 |
| 1653 | 1653 | static constexpr ngtcp2_callbacks CLIENT = { | 1654 | 1654 | ngtcp2_crypto_client_initial_cb, | 1655 | | - nullptr, | | 1655 | + nullptr, // stream_stop_sending | 1656 | 1656 | ngtcp2_crypto_recv_crypto_data_cb, | 1657 | 1657 | on_handshake_completed, | 1658 | 1658 | on_receive_version_negotiation, | @@ -1686,7 +1686,7 @@ struct Session::Impl final : public MemoryRetainer { | 1686 | 1686 | on_acknowledge_datagram, | 1687 | 1687 | on_lost_datagram, | 1688 | 1688 | nullptr, // get_path_challenge_data (deprecated, use v2 below) | 1689 | | - on_stream_stop_sending, | | 1689 | + nullptr, // stream_stop_sending | 1690 | 1690 | ngtcp2_crypto_version_negotiation_cb, | 1691 | 1691 | on_receive_rx_key, | 1692 | 1692 | on_receive_tx_key, | @@ -1697,12 +1697,12 @@ struct Session::Impl final : public MemoryRetainer { | 1697 | 1697 | on_cid_status, | 1698 | 1698 | ngtcp2_crypto_get_path_challenge_data2_cb, | 1699 | 1699 | #ifdef NGTCP2_CALLBACKS_V4 | 1700 | | - nullptr, | | 1700 | + on_receive_stream_stop_sending, | 1701 | 1701 | #endif | 1702 | 1702 | }; | 1703 | 1703 |
| 1704 | 1704 | static constexpr ngtcp2_callbacks SERVER = { | 1705 | | - nullptr, | | 1705 | + nullptr, // stream_stop_sending | 1706 | 1706 | ngtcp2_crypto_recv_client_initial_cb, | 1707 | 1707 | ngtcp2_crypto_recv_crypto_data_cb, | 1708 | 1708 | on_handshake_completed, | @@ -1737,7 +1737,7 @@ struct Session::Impl final : public MemoryRetainer { | 1737 | 1737 | on_acknowledge_datagram, | 1738 | 1738 | on_lost_datagram, | 1739 | 1739 | nullptr, // get_path_challenge_data (deprecated, use v2 below) | 1740 | | - on_stream_stop_sending, | | 1740 | + nullptr, // stream_stop_sending | 1741 | 1741 | ngtcp2_crypto_version_negotiation_cb, | 1742 | 1742 | nullptr, | 1743 | 1743 | on_receive_tx_key, | @@ -1748,7 +1748,7 @@ struct Session::Impl final : public MemoryRetainer { | 1748 | 1748 | on_cid_status, | 1749 | 1749 | ngtcp2_crypto_get_path_challenge_data2_cb, | 1750 | 1750 | #ifdef NGTCP2_CALLBACKS_V4 | 1751 | | - nullptr, | | 1751 | + on_receive_stream_stop_sending, | 1752 | 1752 | #endif | 1753 | 1753 | }; | 1754 | 1754 | }; | |
Collapse file Copy file name to clipboardExpand all lines: src/quic/streams.cc +16-11Lines changed: 16 additions & 11 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -61,6 +61,8 @@ namespace quic { | 61 | 61 | V(WANTS_HEADERS, wants_headers, uint8_t) \ | 62 | 62 | /* Set when the stream has a reset event handler */ \ | 63 | 63 | V(WANTS_RESET, wants_reset, uint8_t) \ | | 64 | + /* Set when the stream has a stop sending event handler */ \ | | 65 | + V(WANTS_STOP_SENDING, wants_stop_sending, uint8_t) \ | 64 | 66 | /* Set when the stream has a trailers event handler */ \ | 65 | 67 | V(WANTS_TRAILERS, wants_trailers, uint8_t) \ | 66 | 68 | /* True when 0-RTT early data was received */ \ | @@ -1774,19 +1776,11 @@ void Stream::ReceiveData(const uint8_t* data, | 1774 | 1776 | } | 1775 | 1777 |
| 1776 | 1778 | void Stream::ReceiveStopSending(QuicError error) { | 1777 | | - // STOP_SENDING from the peer asks us to stop sending. Per RFC 9000 | 1778 | | - // §3.5 the receiver SHOULD respond with RESET_STREAM, which is what | 1779 | | - // ngtcp2_conn_shutdown_stream_write below schedules. If our | 1780 | | - // writable side has already been shut down (e.g. we already sent | 1781 | | - // RESET_STREAM ourselves or finished sending with FIN) there is | 1782 | | - // nothing more to do here. The previous guard checked | 1783 | | - // `state()->read_ended` which is unrelated to the writable side and | 1784 | | - // suppressed STOP_SENDING handling whenever a sibling RESET_STREAM | 1785 | | - // frame had been processed first within the same packet. | 1786 | | - if (state()->write_ended) return; | | 1779 | + // STOP_SENDING from the peer asks us to stop sending. The required | | 1780 | + // RESET_STREAM response is scheduled automatically. | 1787 | 1781 | Debug(this, "Received stop sending with error %s", error); | 1788 | | - ngtcp2_conn_shutdown_stream_write(session(), 0, id(), error.code()); | 1789 | 1782 | EndWritable(); | | 1783 | + EmitStopSending(error); | 1790 | 1784 | } | 1791 | 1785 |
| 1792 | 1786 | void Stream::ReceiveStreamReset(uint64_t final_size, QuicError error) { | @@ -1958,6 +1952,17 @@ void Stream::EmitReset(const QuicError& error) { | 1958 | 1952 | MakeCallback(BindingData::Get(env()).stream_reset_callback(), 1, &err); | 1959 | 1953 | } | 1960 | 1954 |
| | 1955 | +void Stream::EmitStopSending(const QuicError& error) { | | 1956 | + if (!env()->can_call_into_js() || !state()->wants_stop_sending) { | | 1957 | + return; | | 1958 | + } | | 1959 | + CallbackScope<Stream> cb_scope(this); | | 1960 | + Local<Value> err; | | 1961 | + if (!error.ToV8Value(env()).ToLocal(&err)) return; | | 1962 | + | | 1963 | + MakeCallback(BindingData::Get(env()).stream_stop_sending_callback(), 1, &err); | | 1964 | +} | | 1965 | + | 1961 | 1966 | void Stream::EmitWantTrailers() { | 1962 | 1967 | // state()->wants_trailers will be set from the javascript side if the | 1963 | 1968 | // stream object has a handler for the trailers event. | |
Collapse file Copy file name to clipboardExpand all lines: src/quic/streams.h +3Lines changed: 3 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -417,6 +417,9 @@ class Stream final : public AsyncWrap, | 417 | 417 | // Notifies the JavaScript side that the stream has been reset. | 418 | 418 | void EmitReset(const QuicError& error); | 419 | 419 |
| | 420 | + // Notifies the JavaScript side that the peer asked it to stop sending. | | 421 | + void EmitStopSending(const QuicError& error); | | 422 | + | 420 | 423 | // Notifies the JavaScript side that the application is ready to receive | 421 | 424 | // trailing headers. Any trailing headers must be sent immediately, and | 422 | 425 | // synchronously when this callback is triggered. | |
Collapse file Copy file name to clipboardExpand all lines: test/parallel/test-quic-internal-endpoint-stats-state.mjs +1Lines changed: 1 addition & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -156,6 +156,7 @@ assert.strictEqual(streamState.reset, false); | 156 | 156 | assert.strictEqual(streamState.hasReader, false); | 157 | 157 | assert.strictEqual(streamState.wantsBlock, false); | 158 | 158 | assert.strictEqual(streamState.wantsReset, false); | | 159 | +assert.strictEqual(streamState.wantsStopSending, false); | 159 | 160 |
| 160 | 161 | assert.strictEqual(sessionState.hasPathValidationListener, false); | 161 | 162 | assert.strictEqual(sessionState.hasDatagramListener, false); | |
|