From b1750a4d533f73fd16c7c3602aebfad0381747e4 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 30 Jun 2020 19:42:52 -0700 Subject: [PATCH 01/25] quic: continued refactoring for quic_stream/quic_session PR-URL: https://github.com/nodejs/node/pull/34160 Reviewed-By: Anna Henningsen --- src/quic/node_quic_default_application.cc | 5 +++- src/quic/node_quic_http3_application.cc | 5 +++- src/quic/node_quic_session.cc | 5 ---- src/quic/node_quic_session.h | 31 ----------------------- src/quic/node_quic_stream-inl.h | 28 +++++++++++++++----- src/quic/node_quic_stream.cc | 31 ++++++++++++++++++++--- src/quic/node_quic_stream.h | 6 +++-- test/parallel/test-quic-statelessreset.js | 3 ++- 8 files changed, 63 insertions(+), 51 deletions(-) diff --git a/src/quic/node_quic_default_application.cc b/src/quic/node_quic_default_application.cc index ea84099d70ae61..4c253b3787727b 100644 --- a/src/quic/node_quic_default_application.cc +++ b/src/quic/node_quic_default_application.cc @@ -93,7 +93,10 @@ bool DefaultApplication::ReceiveStreamData( if (!stream) { // Shutdown the stream explicitly if the session is being closed. if (session()->is_gracefully_closing()) { - session()->ResetStream(stream_id, NGTCP2_ERR_CLOSING); + ngtcp2_conn_shutdown_stream( + session()->connection(), + stream_id, + NGTCP2_ERR_CLOSING); return true; } diff --git a/src/quic/node_quic_http3_application.cc b/src/quic/node_quic_http3_application.cc index 673e8a4eace9ea..44f874cf7aeb8f 100644 --- a/src/quic/node_quic_http3_application.cc +++ b/src/quic/node_quic_http3_application.cc @@ -702,7 +702,10 @@ void Http3Application::PushStream( void Http3Application::SendStopSending( int64_t stream_id, uint64_t app_error_code) { - session()->ResetStream(stream_id, app_error_code); + ngtcp2_conn_shutdown_stream_read( + session()->connection(), + stream_id, + app_error_code); } void Http3Application::EndStream(int64_t stream_id) { diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index d058035a0b7896..d513b163cda7f1 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -2328,11 +2328,6 @@ void QuicSession::ResumeStream(int64_t stream_id) { application()->ResumeStream(stream_id); } -void QuicSession::ResetStream(int64_t stream_id, uint64_t code) { - SendSessionScope scope(this); - CHECK_EQ(ngtcp2_conn_shutdown_stream(connection(), stream_id, code), 0); -} - // Silent Close must start with the JavaScript side, which must // clean up state, abort any still existing QuicSessions, then // destroy the handle when done. The most important characteristic diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index 78fb7ee0a30540..1e9341beecfe05 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -957,37 +957,6 @@ class QuicSession : public AsyncWrap, const StreamsMap& streams() const { return streams_; } - // ResetStream will cause ngtcp2 to queue a - // RESET_STREAM and STOP_SENDING frame, as appropriate, - // for the given stream_id. For a locally-initiated - // unidirectional stream, only a RESET_STREAM frame - // will be scheduled and the stream will be immediately - // closed. For a bi-directional stream, a STOP_SENDING - // frame will be sent. - // - // It is important to note that the QuicStream is - // not destroyed immediately following ShutdownStream. - // The sending QuicSession will not close the stream - // until the RESET_STREAM is acknowledged. - // - // Once the RESET_STREAM is sent, the QuicSession - // should not send any new frames for the stream, - // and all inbound stream frames should be discarded. - // Once ngtcp2 receives the appropriate notification - // that the RESET_STREAM has been acknowledged, the - // stream will be closed. - // - // Once the stream has been closed, it will be - // destroyed and memory will be freed. User code - // can request that a stream be immediately and - // abruptly destroyed without calling ShutdownStream. - // Likewise, an idle timeout may cause the stream - // to be silently destroyed without calling - // ShutdownStream. - void ResetStream( - int64_t stream_id, - uint64_t error_code = NGTCP2_APP_NOERROR); - void ResumeStream(int64_t stream_id); // Submits informational headers to the QUIC Application diff --git a/src/quic/node_quic_stream-inl.h b/src/quic/node_quic_stream-inl.h index 3dd1fc216d9783..546e867ee2db7e 100644 --- a/src/quic/node_quic_stream-inl.h +++ b/src/quic/node_quic_stream-inl.h @@ -132,19 +132,33 @@ void QuicStream::Commit(size_t amount) { streambuf_.Seek(amount); } +// ResetStream will cause ngtcp2 to queue a RESET_STREAM and STOP_SENDING +// frame, as appropriate, for the given stream_id. For a locally-initiated +// unidirectional stream, only a RESET_STREAM frame will be scheduled and +// the stream will be immediately closed. For a bidirectional stream, a +// STOP_SENDING frame will be sent. void QuicStream::ResetStream(uint64_t app_error_code) { - // On calling shutdown, the stream will no longer be - // readable or writable, all any pending data in the - // streambuf_ will be canceled, and all data pending - // to be acknowledged at the ngtcp2 level will be - // abandoned. - BaseObjectPtr ptr(session_); + QuicSession::SendSessionScope send_scope(session()); + ngtcp2_conn_shutdown_stream( + session()->connection(), + stream_id_, + app_error_code); set_flag(QUICSTREAM_FLAG_READ_CLOSED); - session_->ResetStream(stream_id_, app_error_code); streambuf_.Cancel(); streambuf_.End(); } +// StopSending will cause ngtcp2 to queue a STOP_SENDING frame if the +// stream is still inbound readable. +void QuicStream::StopSending(uint64_t app_error_code) { + QuicSession::SendSessionScope send_scope(session()); + ngtcp2_conn_shutdown_stream_read( + session()->connection(), + stream_id_, + app_error_code); + set_flag(QUICSTREAM_FLAG_READ_CLOSED); +} + void QuicStream::Schedule(Queue* queue) { if (!stream_queue_.IsEmpty()) // Already scheduled? return; diff --git a/src/quic/node_quic_stream.cc b/src/quic/node_quic_stream.cc index ce8cc78a1ec8c5..ba447e760d206a 100644 --- a/src/quic/node_quic_stream.cc +++ b/src/quic/node_quic_stream.cc @@ -118,21 +118,31 @@ std::string QuicStream::diagnostic_name() const { ", " + session_->diagnostic_name() + ")"; } -void QuicStream::Destroy() { +void QuicStream::Destroy(QuicError* error) { if (is_destroyed()) return; + + QuicSession::SendSessionScope send_scope(session()); + set_flag(QUICSTREAM_FLAG_DESTROYED); set_flag(QUICSTREAM_FLAG_READ_CLOSED); - streambuf_.End(); + + // In case this stream is scheduled for sending, remove it + // from the schedule queue + Unschedule(); // If there is data currently buffered in the streambuf_, // then cancel will call out to invoke an arbitrary // JavaScript callback (the on write callback). Within // that callback, however, the QuicStream will no longer // be usable to send or receive data. + streambuf_.End(); streambuf_.Cancel(); CHECK_EQ(streambuf_.length(), 0); + // Attempt to send a shutdown signal to the remote peer + ResetStream(error != nullptr ? error->code : NGTCP2_NO_ERROR); + // The QuicSession maintains a map of std::unique_ptrs to // QuicStream instances. Removing this here will cause // this QuicStream object to be deconstructed, so the @@ -411,9 +421,11 @@ void OpenBidirectionalStream(const FunctionCallbackInfo& args) { } void QuicStreamDestroy(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); QuicStream* stream; ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); - stream->Destroy(); + QuicError error(env, args[0], args[1], QUIC_ERROR_APPLICATION); + stream->Destroy(&error); } void QuicStreamReset(const FunctionCallbackInfo& args) { @@ -428,6 +440,18 @@ void QuicStreamReset(const FunctionCallbackInfo& args) { error.code : static_cast(NGTCP2_NO_ERROR)); } +void QuicStreamStopSending(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + QuicStream* stream; + ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); + + QuicError error(env, args[0], args[1], QUIC_ERROR_APPLICATION); + + stream->StopSending( + error.family == QUIC_ERROR_APPLICATION ? + error.code : static_cast(NGTCP2_NO_ERROR)); +} + // Requests transmission of a block of informational headers. Not all // QUIC Applications will support headers. If headers are not supported, // This will set the return value to false, otherwise the return value @@ -494,6 +518,7 @@ void QuicStream::Initialize( streamt->Set(env->owner_symbol(), Null(env->isolate())); env->SetProtoMethod(stream, "destroy", QuicStreamDestroy); env->SetProtoMethod(stream, "resetStream", QuicStreamReset); + env->SetProtoMethod(stream, "stopSending", QuicStreamStopSending); env->SetProtoMethod(stream, "id", QuicStreamGetID); env->SetProtoMethod(stream, "submitInformation", QuicStreamSubmitInformation); env->SetProtoMethod(stream, "submitHeaders", QuicStreamSubmitHeaders); diff --git a/src/quic/node_quic_stream.h b/src/quic/node_quic_stream.h index d8297f300ba85c..0d0809c74df0ed 100644 --- a/src/quic/node_quic_stream.h +++ b/src/quic/node_quic_stream.h @@ -275,7 +275,7 @@ class QuicStream : public AsyncWrap, void Acknowledge(uint64_t offset, size_t datalen); // Destroy the QuicStream and render it no longer usable. - void Destroy(); + void Destroy(QuicError* error = nullptr); // Buffers chunks of data to be written to the QUIC connection. int DoWrite( @@ -312,7 +312,9 @@ class QuicStream : public AsyncWrap, // Resets the QUIC stream, sending a signal to the peer that // no additional data will be transmitted for this stream. - inline void ResetStream(uint64_t app_error_code = 0); + inline void ResetStream(uint64_t app_error_code = NGTCP2_NO_ERROR); + + inline void StopSending(uint64_t app_error_code = NGTCP2_NO_ERROR); // Submits informational headers. Returns false if headers are not // supported on the underlying QuicApplication. diff --git a/test/parallel/test-quic-statelessreset.js b/test/parallel/test-quic-statelessreset.js index b8d0279660c3a5..370fbf15de9acf 100644 --- a/test/parallel/test-quic-statelessreset.js +++ b/test/parallel/test-quic-statelessreset.js @@ -44,7 +44,8 @@ server.on('session', common.mustCall((session) => { server.on('close', common.mustCall(() => { // Verify stats recording - assert.strictEqual(server.statelessResetCount, 1n); + console.log(server.statelessResetCount); + assert(server.statelessResetCount >= 1n); })); server.on('ready', common.mustCall(() => { From b5bf5bb20f35372a81a83c830a01353925e11701 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 1 Jul 2020 07:29:19 -0700 Subject: [PATCH 02/25] quic: refactor native object flags for better readability Use is_* and set_* pattern for native object flags to improve readability in the code. PR-URL: https://github.com/nodejs/node/pull/34160 Reviewed-By: Anna Henningsen --- lib/internal/quic/core.js | 8 +- src/quic/node_quic_default_application.cc | 2 +- src/quic/node_quic_http3_application.cc | 2 +- src/quic/node_quic_session-inl.h | 43 +++---- src/quic/node_quic_session.cc | 110 +++++++++--------- src/quic/node_quic_session.h | 134 ++++++++++------------ src/quic/node_quic_socket-inl.h | 20 ++-- src/quic/node_quic_socket.cc | 15 ++- src/quic/node_quic_socket.h | 87 +++++++------- src/quic/node_quic_stream-inl.h | 33 +----- src/quic/node_quic_stream.cc | 12 +- src/quic/node_quic_stream.h | 67 +++++------ 12 files changed, 234 insertions(+), 299 deletions(-) diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index 446bec3761374b..90727194246450 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -966,9 +966,11 @@ class QuicSocket extends EventEmitter { state.lookup = lookup || (type === AF_INET6 ? lookup6 : lookup4); state.server = server; - const socketOptions = - (validateAddress ? QUICSOCKET_OPTIONS_VALIDATE_ADDRESS : 0) | - (validateAddressLRU ? QUICSOCKET_OPTIONS_VALIDATE_ADDRESS_LRU : 0); + let socketOptions = 0; + if (validateAddress) + socketOptions |= (1 << QUICSOCKET_OPTIONS_VALIDATE_ADDRESS); + if (validateAddressLRU) + socketOptions |= (1 << QUICSOCKET_OPTIONS_VALIDATE_ADDRESS_LRU); this[kSetHandle]( new QuicSocketHandle( diff --git a/src/quic/node_quic_default_application.cc b/src/quic/node_quic_default_application.cc index 4c253b3787727b..5af8dc098d6027 100644 --- a/src/quic/node_quic_default_application.cc +++ b/src/quic/node_quic_default_application.cc @@ -92,7 +92,7 @@ bool DefaultApplication::ReceiveStreamData( BaseObjectPtr stream = session()->FindStream(stream_id); if (!stream) { // Shutdown the stream explicitly if the session is being closed. - if (session()->is_gracefully_closing()) { + if (session()->is_graceful_closing()) { ngtcp2_conn_shutdown_stream( session()->connection(), stream_id, diff --git a/src/quic/node_quic_http3_application.cc b/src/quic/node_quic_http3_application.cc index 44f874cf7aeb8f..7dc41a392e54df 100644 --- a/src/quic/node_quic_http3_application.cc +++ b/src/quic/node_quic_http3_application.cc @@ -603,7 +603,7 @@ BaseObjectPtr Http3Application::FindOrCreateStream( int64_t stream_id) { BaseObjectPtr stream = session()->FindStream(stream_id); if (!stream) { - if (session()->is_gracefully_closing()) { + if (session()->is_graceful_closing()) { nghttp3_conn_close_stream(connection(), stream_id, NGTCP2_ERR_CLOSING); return {}; } diff --git a/src/quic/node_quic_session-inl.h b/src/quic/node_quic_session-inl.h index 5acb8a2e9350ef..339df8210f4a09 100644 --- a/src/quic/node_quic_session-inl.h +++ b/src/quic/node_quic_session-inl.h @@ -112,7 +112,10 @@ void QuicCryptoContext::Keylog(const char* line) { void QuicCryptoContext::OnClientHelloDone() { // Continue the TLS handshake when this function exits // otherwise it will stall and fail. - TLSHandshakeScope handshake(this, &in_client_hello_); + TLSHandshakeScope handshake_scope( + this, + [&]() { set_in_client_hello(false); }); + // Disable the callback at this point so we don't loop continuously session_->state_[IDX_QUIC_SESSION_STATE_CLIENT_HELLO_ENABLED] = 0; } @@ -129,8 +132,8 @@ void QuicCryptoContext::ResumeHandshake() { // For 0RTT, this sets the TLS session data from the given buffer. bool QuicCryptoContext::set_session(crypto::SSLSessionPointer session) { if (side_ == NGTCP2_CRYPTO_SIDE_CLIENT && session != nullptr) { - early_data_ = - SSL_SESSION_get_max_early_data(session.get()) == 0xffffffffUL; + set_early_data( + SSL_SESSION_get_max_early_data(session.get()) == 0xffffffffUL); } return crypto::SetTLSSession(ssl_, std::move(session)); } @@ -186,7 +189,7 @@ std::string QuicCryptoContext::selected_alpn() const { bool QuicCryptoContext::early_data() const { return - (early_data_ && + (is_early_data() && SSL_get_early_data_status(ssl_.get()) == SSL_EARLY_DATA_ACCEPTED) || SSL_get_max_early_data(ssl_.get()) == 0xffffffffUL; } @@ -300,13 +303,13 @@ void QuicSession::ExtendOffset(size_t amount) { // Copies the local transport params into the given struct for serialization. void QuicSession::GetLocalTransportParams(ngtcp2_transport_params* params) { - CHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); + CHECK(!is_destroyed()); ngtcp2_conn_get_local_transport_params(connection(), params); } // Gets the QUIC version negotiated for this QuicSession uint32_t QuicSession::negotiated_version() const { - CHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); + CHECK(!is_destroyed()); return ngtcp2_conn_get_negotiated_version(connection()); } @@ -328,7 +331,7 @@ void QuicSession::HandshakeConfirmed() { } bool QuicSession::is_handshake_completed() const { - DCHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); + DCHECK(!is_destroyed()); return ngtcp2_conn_get_handshake_completed(connection()); } @@ -342,7 +345,7 @@ void QuicSession::InitApplication() { // immediately closed without attempting to send any additional data to // the peer. All existing streams are abandoned and closed. void QuicSession::OnIdleTimeout() { - if (!is_flag_set(QUICSESSION_FLAG_DESTROYED)) { + if (!is_destroyed()) { state_[IDX_QUIC_SESSION_STATE_IDLE_TIMEOUT] = 1; Debug(this, "Idle timeout"); SilentClose(); @@ -359,7 +362,7 @@ void QuicSession::GetConnectionCloseInfo() { // Removes the given connection id from the QuicSession. void QuicSession::RemoveConnectionID(const QuicCID& cid) { - if (!is_flag_set(QUICSESSION_FLAG_DESTROYED)) + if (!is_destroyed()) DisassociateCID(cid); } @@ -440,24 +443,12 @@ SessionTicketAppData::Status QuicSession::GetSessionTicketAppData( return application_->GetSessionTicketAppData(app_data, flag); } -bool QuicSession::is_gracefully_closing() const { - return is_flag_set(QUICSESSION_FLAG_GRACEFUL_CLOSING); -} - -bool QuicSession::is_destroyed() const { - return is_flag_set(QUICSESSION_FLAG_DESTROYED); -} - -bool QuicSession::is_stateless_reset() const { - return is_flag_set(QUICSESSION_FLAG_STATELESS_RESET); -} - bool QuicSession::is_server() const { return crypto_context_->side() == NGTCP2_CRYPTO_SIDE_SERVER; } void QuicSession::StartGracefulClose() { - set_flag(QUICSESSION_FLAG_GRACEFUL_CLOSING); + set_graceful_closing(); RecordTimestamp(&QuicSessionStats::closing_at); } @@ -511,15 +502,15 @@ bool QuicSession::SendPacket( } void QuicSession::set_local_address(const ngtcp2_addr* addr) { - DCHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); + DCHECK(!is_destroyed()); ngtcp2_conn_set_local_addr(connection(), addr); } // Set the transport parameters received from the remote peer void QuicSession::set_remote_transport_params() { - DCHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); + DCHECK(!is_destroyed()); ngtcp2_conn_get_remote_transport_params(connection(), &transport_params_); - set_flag(QUICSESSION_FLAG_HAS_TRANSPORT_PARAMS); + set_transport_params_set(); } void QuicSession::StopIdleTimer() { @@ -537,7 +528,7 @@ void QuicSession::StopRetransmitTimer() { // parameter is an array of versions supported by the remote peer. void QuicSession::VersionNegotiation(const uint32_t* sv, size_t nsv) { CHECK(!is_server()); - if (!is_flag_set(QUICSESSION_FLAG_DESTROYED)) + if (!is_destroyed()) listener()->OnVersionNegotiation(NGTCP2_PROTO_VER, sv, nsv); } diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index d513b163cda7f1..d28f97cdba8fb8 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -648,8 +648,7 @@ void JSQuicSessionListener::OnSessionTicket(int size, SSL_SESSION* sess) { argv[0] = session_ticket.ToBuffer().ToLocalChecked(); } - if (session()->is_flag_set( - QuicSession::QUICSESSION_FLAG_HAS_TRANSPORT_PARAMS)) { + if (session()->is_transport_params_set()) { argv[1] = Buffer::Copy( env, reinterpret_cast(&session()->transport_params_), @@ -923,15 +922,9 @@ int QuicCryptoContext::OnClientHello() { TLSCallbackScope callback_scope(this); - // Not an error but does suspend the handshake until we're ready to go. - // A callback function is passed to the JavaScript function below that - // must be called in order to turn QUICSESSION_FLAG_CLIENT_HELLO_CB_RUNNING - // off. Once that callback is invoked, the TLS Handshake will resume. - // It is recommended that the user not take a long time to invoke the - // callback in order to avoid stalling out the QUIC connection. - if (in_client_hello_) + if (is_in_client_hello()) return -1; - in_client_hello_ = true; + set_in_client_hello(); QuicCryptoContext* ctx = session_->crypto_context(); session_->listener()->OnClientHello( @@ -943,7 +936,7 @@ int QuicCryptoContext::OnClientHello() { // handshake is ready to proceed. When the OnClientHello callback // is called above, it may be resolved synchronously or asynchronously. // In case it is resolved synchronously, we need the check below. - return in_client_hello_ ? -1 : 0; + return is_in_client_hello() ? -1 : 0; } // The OnCert callback provides an opportunity to prompt the server to @@ -965,9 +958,9 @@ int QuicCryptoContext::OnOCSP() { // As in node_crypto.cc, this is not an error, but does suspend the // handshake to continue when OnOCSP is complete. - if (in_ocsp_request_) + if (is_in_ocsp_request()) return -1; - in_ocsp_request_ = true; + set_in_ocsp_request(); session_->listener()->OnCert(session_->crypto_context()->servername()); @@ -975,7 +968,7 @@ int QuicCryptoContext::OnOCSP() { // request to be completed. When the OnCert handler is invoked // above, it can be resolve synchronously or asynchonously. If // resolved synchronously, we need the check below. - return in_ocsp_request_ ? -1 : 1; + return is_in_ocsp_request() ? -1 : 1; } // The OnCertDone function is called by the QuicSessionOnCertDone @@ -989,7 +982,9 @@ void QuicCryptoContext::OnOCSPDone( ocsp_response->IsArrayBufferView() ? "Yes" : "No"); // Continue the TLS handshake when this function exits // otherwise it will stall and fail. - TLSHandshakeScope handshake_scope(this, &in_ocsp_request_); + TLSHandshakeScope handshake_scope( + this, + [&]() { set_in_ocsp_request(false); }); // Disable the callback at this point so we don't loop continuously session_->state_[IDX_QUIC_SESSION_STATE_CERT_ENABLED] = 0; @@ -1137,9 +1132,9 @@ bool QuicCryptoContext::InitiateKeyUpdate() { // There's no user code that should be able to run while UpdateKey // is running, but we need to gate on it just to be safe. - auto leave = OnScopeLeave([&]() { in_key_update_ = false; }); - CHECK(!in_key_update_); - in_key_update_ = true; + auto leave = OnScopeLeave([&]() { set_in_key_update(false); }); + CHECK(!is_in_key_update()); + set_in_key_update(); Debug(session(), "Initiating Key Update"); session_->IncrementStat(&QuicSessionStats::keyupdate_count); @@ -1549,7 +1544,7 @@ void QuicSession::AckedStreamDataOffset( // It is possible for the QuicSession to have been destroyed but not yet // deconstructed. In such cases, we want to ignore the callback as there // is nothing to do but wait for further cleanup to happen. - if (LIKELY(!is_flag_set(QUICSESSION_FLAG_DESTROYED))) { + if (LIKELY(!is_destroyed())) { Debug(this, "Received acknowledgement for %" PRIu64 " bytes of stream %" PRId64 " data", @@ -1602,7 +1597,7 @@ void QuicSession::AddToSocket(QuicSocket* socket) { // Add the given QuicStream to this QuicSession's collection of streams. All // streams added must be removed before the QuicSession instance is freed. void QuicSession::AddStream(BaseObjectPtr stream) { - DCHECK(!is_flag_set(QUICSESSION_FLAG_GRACEFUL_CLOSING)); + DCHECK(!is_graceful_closing()); Debug(this, "Adding stream %" PRId64 " to session", stream->id()); streams_.emplace(stream->id(), stream); @@ -1642,9 +1637,9 @@ void QuicSession::AddStream(BaseObjectPtr stream) { // not immediately torn down, but is allowed to drain // properly per the QUIC spec description of "immediate close". void QuicSession::ImmediateClose() { - if (is_flag_set(QUICSESSION_FLAG_CLOSING)) + if (is_closing() || is_silent_closing()) return; - set_flag(QUICSESSION_FLAG_CLOSING); + set_closing(); QuicError err = last_error(); Debug(this, "Immediate close with code %" PRIu64 " (%s)", @@ -1657,9 +1652,9 @@ void QuicSession::ImmediateClose() { // Creates a new stream object and passes it off to the javascript side. // This has to be called from within a handlescope/contextscope. BaseObjectPtr QuicSession::CreateStream(int64_t stream_id) { - CHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); - CHECK(!is_flag_set(QUICSESSION_FLAG_GRACEFUL_CLOSING)); - CHECK(!is_flag_set(QUICSESSION_FLAG_CLOSING)); + CHECK(!is_destroyed()); + CHECK(!is_graceful_closing()); + CHECK(!is_closing()); BaseObjectPtr stream = QuicStream::New(this, stream_id); CHECK(stream); @@ -1671,7 +1666,7 @@ BaseObjectPtr QuicSession::CreateStream(int64_t stream_id) { // the QuicSession instance will be generally unusable but most // likely will not be immediately freed. void QuicSession::Destroy() { - if (is_flag_set(QUICSESSION_FLAG_DESTROYED)) + if (is_destroyed()) return; // If we're not in the closing or draining periods, @@ -1689,9 +1684,9 @@ void QuicSession::Destroy() { CHECK(streams_.empty()); // Mark the session destroyed. - set_flag(QUICSESSION_FLAG_DESTROYED); - set_flag(QUICSESSION_FLAG_CLOSING, false); - set_flag(QUICSESSION_FLAG_GRACEFUL_CLOSING, false); + set_destroyed(); + set_closing(false); + set_graceful_closing(false); // Stop and free the idle and retransmission timers if they are active. StopIdleTimer(); @@ -1714,9 +1709,8 @@ bool QuicSession::GetNewConnectionID( ngtcp2_cid* cid, uint8_t* token, size_t cidlen) { - if (is_flag_set(QUICSESSION_FLAG_DESTROYED)) + if (is_destroyed()) return false; - CHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); CHECK_NOT_NULL(connection_id_strategy_); connection_id_strategy_(this, cid, cidlen); QuicCID cid_(cid); @@ -1739,7 +1733,7 @@ void QuicSession::HandleError() { // which determines whether or not we need to retransmit data to // to packet loss or ack delay. void QuicSession::MaybeTimeout() { - if (is_flag_set(QUICSESSION_FLAG_DESTROYED)) + if (is_destroyed()) return; uint64_t now = uv_hrtime(); bool transmit = false; @@ -1759,16 +1753,16 @@ void QuicSession::MaybeTimeout() { } bool QuicSession::OpenBidirectionalStream(int64_t* stream_id) { - DCHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); - DCHECK(!is_flag_set(QUICSESSION_FLAG_CLOSING)); - DCHECK(!is_flag_set(QUICSESSION_FLAG_GRACEFUL_CLOSING)); + DCHECK(!is_destroyed()); + DCHECK(!is_closing()); + DCHECK(!is_graceful_closing()); return ngtcp2_conn_open_bidi_stream(connection(), stream_id, nullptr) == 0; } bool QuicSession::OpenUnidirectionalStream(int64_t* stream_id) { - DCHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); - DCHECK(!is_flag_set(QUICSESSION_FLAG_CLOSING)); - DCHECK(!is_flag_set(QUICSESSION_FLAG_GRACEFUL_CLOSING)); + DCHECK(!is_destroyed()); + DCHECK(!is_closing()); + DCHECK(!is_graceful_closing()); if (ngtcp2_conn_open_uni_stream(connection(), stream_id, nullptr)) return false; ngtcp2_conn_shutdown_stream_read(connection(), *stream_id, 0); @@ -1808,8 +1802,8 @@ void QuicSession::PathValidation( // closing or draining period has started, this is a non-op. void QuicSession::Ping() { if (Ngtcp2CallbackScope::InNgtcp2CallbackScope(this) || - is_flag_set(QUICSESSION_FLAG_DESTROYED) || - is_flag_set(QUICSESSION_FLAG_CLOSING) || + is_destroyed() || + is_closing() || is_in_closing_period() || is_in_draining_period()) { return; @@ -1830,7 +1824,7 @@ bool QuicSession::Receive( const SocketAddress& local_addr, const SocketAddress& remote_addr, unsigned int flags) { - if (is_flag_set(QUICSESSION_FLAG_DESTROYED)) { + if (is_destroyed()) { Debug(this, "Ignoring packet because session is destroyed"); return false; } @@ -1840,7 +1834,7 @@ bool QuicSession::Receive( // Closing period starts once ngtcp2 has detected that the session // is being shutdown locally. Note that this is different that the - // is_flag_set(QUICSESSION_FLAG_GRACEFUL_CLOSING) function, which + // is_graceful_closing() function, which // indicates a graceful shutdown that allows the session and streams // to finish naturally. When is_in_closing_period is true, ngtcp2 is // actively in the process of shutting down the connection and a @@ -1944,7 +1938,7 @@ bool QuicSession::ReceivePacket( // If the QuicSession has been destroyed, we're not going // to process any more packets for it. - if (is_flag_set(QUICSESSION_FLAG_DESTROYED)) + if (is_destroyed()) return true; uint64_t now = uv_hrtime(); @@ -2001,7 +1995,7 @@ bool QuicSession::ReceiveStreamData( if (UNLIKELY(!(flags & NGTCP2_STREAM_DATA_FLAG_FIN) && datalen == 0)) return true; - if (is_flag_set(QUICSESSION_FLAG_DESTROYED)) + if (is_destroyed()) return false; HandleScope scope(env()->isolate()); @@ -2101,7 +2095,7 @@ bool QuicSession::SendConnectionClose() { // Do not send any frames at all if we're in the draining period // or in the middle of a silent close - if (is_in_draining_period() || is_flag_set(QUICSESSION_FLAG_SILENT_CLOSE)) + if (is_in_draining_period() || is_silent_closing()) return true; // The specific handling of connection close varies for client @@ -2204,7 +2198,7 @@ void QuicSession::UsePreferredAddressStrategy( // Passes a serialized packet to the associated QuicSocket. bool QuicSession::SendPacket(std::unique_ptr packet) { - CHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); + CHECK(!is_destroyed()); CHECK(!is_in_draining_period()); // There's nothing to send. @@ -2262,7 +2256,7 @@ void QuicSession::SendPendingData() { // session resumption. int QuicSession::set_session(SSL_SESSION* session) { CHECK(!is_server()); - CHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); + CHECK(!is_destroyed()); int size = i2d_SSL_SESSION(session, nullptr); if (size > SecureContext::kMaxSessionSize) return 0; @@ -2274,9 +2268,9 @@ int QuicSession::set_session(SSL_SESSION* session) { // TODO(@jasnell): This will be revisited. bool QuicSession::set_socket(QuicSocket* socket, bool nat_rebinding) { CHECK(!is_server()); - CHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); + CHECK(!is_destroyed()); - if (is_flag_set(QUICSESSION_FLAG_GRACEFUL_CLOSING)) + if (is_graceful_closing()) return false; if (socket == nullptr || socket == socket_.get()) @@ -2340,9 +2334,9 @@ void QuicSession::ResumeStream(int64_t stream_id) { // notify the JavaScript side and destroy the connection with // a flag set that indicates stateless reset. void QuicSession::SilentClose() { - CHECK(!is_flag_set(QUICSESSION_FLAG_SILENT_CLOSE)); - set_flag(QUICSESSION_FLAG_SILENT_CLOSE); - set_flag(QUICSESSION_FLAG_CLOSING); + CHECK(!is_silent_closing()); + set_silent_closing(); + set_closing(); QuicError err = last_error(); Debug(this, @@ -2364,7 +2358,7 @@ void QuicSession::SilentClose() { // multiple times. On client QuicSession instances, we only ever // serialize the connection close once. bool QuicSession::StartClosingPeriod() { - if (is_flag_set(QUICSESSION_FLAG_DESTROYED)) + if (is_destroyed()) return false; if (is_in_closing_period()) return true; @@ -2403,7 +2397,7 @@ bool QuicSession::StartClosingPeriod() { // Called by ngtcp2 when a stream has been closed. If the stream does // not exist, the close is ignored. void QuicSession::StreamClose(int64_t stream_id, uint64_t app_error_code) { - if (is_flag_set(QUICSESSION_FLAG_DESTROYED)) + if (is_destroyed()) return; Debug(this, "Closing stream %" PRId64 " with code %" PRIu64, @@ -2419,7 +2413,7 @@ void QuicSession::StreamClose(int64_t stream_id, uint64_t app_error_code) { // a stream commitment attack. The only exception is shutting the // stream down explicitly if we are in a graceful close period. void QuicSession::StreamOpen(int64_t stream_id) { - if (is_flag_set(QUICSESSION_FLAG_GRACEFUL_CLOSING)) { + if (is_graceful_closing()) { ngtcp2_conn_shutdown_stream( connection(), stream_id, @@ -2451,7 +2445,7 @@ void QuicSession::StreamReset( int64_t stream_id, uint64_t final_size, uint64_t app_error_code) { - if (is_flag_set(QUICSESSION_FLAG_DESTROYED)) + if (is_destroyed()) return; Debug(this, @@ -2514,7 +2508,7 @@ void QuicSession::UpdateIdleTimer() { // serialize stream data that is being sent initially. bool QuicSession::WritePackets(const char* diagnostic_label) { CHECK(!Ngtcp2CallbackScope::InNgtcp2CallbackScope(this)); - CHECK(!is_flag_set(QUICSESSION_FLAG_DESTROYED)); + CHECK(!is_destroyed()); // During the draining period, we must not send any frames at all. if (is_in_draining_period()) @@ -3269,7 +3263,7 @@ int QuicSession::OnStatelessReset( QuicSession* session = static_cast(user_data); if (UNLIKELY(session->is_destroyed())) return NGTCP2_ERR_CALLBACK_FAILURE; - session->set_flag(QUICSESSION_FLAG_STATELESS_RESET); + session->set_stateless_reset(); return 0; } diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index 1e9341beecfe05..8c22fe3bdab534 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -358,6 +358,13 @@ class JSQuicSessionListener : public QuicSessionListener { friend class QuicSession; }; +#define QUICCRYPTOCONTEXT_FLAGS(V) \ + V(IN_TLS_CALLBACK, in_tls_callback) \ + V(IN_KEY_UPDATE, in_key_update) \ + V(IN_OCSP_RESPONSE, in_ocsp_request) \ + V(IN_CLIENT_HELLO, in_client_hello) \ + V(EARLY_DATA, early_data) + // The QuicCryptoContext class encapsulates all of the crypto/TLS // handshake details on behalf of a QuicSession. class QuicCryptoContext : public MemoryRetainer { @@ -440,6 +447,18 @@ class QuicCryptoContext : public MemoryRetainer { options_ &= ~option; } +#define V(id, name) \ + inline bool is_##name() const { \ + return flags_ & (1 << QUICCRYPTOCONTEXT_FLAG_##id); } \ + inline void set_##name(bool on = true) { \ + if (on) \ + flags_ |= (1 << QUICCRYPTOCONTEXT_FLAG_##id); \ + else \ + flags_ &= ~(1 << QUICCRYPTOCONTEXT_FLAG_##id); \ + } + QUICCRYPTOCONTEXT_FLAGS(V) +#undef V + inline bool set_session(crypto::SSLSessionPointer session); inline void set_tls_alert(int err); @@ -480,29 +499,32 @@ class QuicCryptoContext : public MemoryRetainer { ngtcp2_crypto_side side_; crypto::SSLPointer ssl_; QuicBuffer handshake_[3]; - bool in_tls_callback_ = false; - bool in_key_update_ = false; - bool in_ocsp_request_ = false; - bool in_client_hello_ = false; - bool early_data_ = false; uint32_t options_; + uint32_t flags_ = 0; v8::Global ocsp_response_; crypto::BIOPointer bio_trace_; +#define V(id, _) QUICCRYPTOCONTEXT_FLAG_##id, + enum QuicCryptoContextFlags : uint32_t { + QUICCRYPTOCONTEXT_FLAGS(V) + QUICCRYPTOCONTEXT_FLAG_COUNT + }; +#undef V + class TLSCallbackScope { public: explicit TLSCallbackScope(QuicCryptoContext* context) : context_(context) { - context_->in_tls_callback_ = true; + context_->set_in_tls_callback(); } ~TLSCallbackScope() { - context_->in_tls_callback_ = false; + context_->set_in_tls_callback(false); } static bool is_in_callback(QuicCryptoContext* context) { - return context->in_tls_callback_; + return context->is_in_tls_callback(); } private: @@ -511,17 +533,18 @@ class QuicCryptoContext : public MemoryRetainer { class TLSHandshakeScope { public: + using DoneCB = std::function; TLSHandshakeScope( QuicCryptoContext* context, - bool* monitor) : + DoneCB done) : context_(context), - monitor_(monitor) {} + done_(done) {} ~TLSHandshakeScope() { if (!is_handshake_suspended()) return; - *monitor_ = false; + done_(); // Only continue the TLS handshake if we are not currently running // synchronously within the TLS handshake function. This can happen // when the callback function passed to the clientHello and cert @@ -533,12 +556,12 @@ class QuicCryptoContext : public MemoryRetainer { private: bool is_handshake_suspended() const { - return context_->in_ocsp_request_ || context_->in_client_hello_; + return context_->is_in_ocsp_request() || context_->is_in_client_hello(); } QuicCryptoContext* context_; - bool* monitor_; + DoneCB done_; }; friend class QuicSession; @@ -663,6 +686,17 @@ class QuicApplication : public MemoryRetainer, size_t max_header_length_ = 0; }; +// QUICSESSION_FLAGS are converted into is_{name}() and set_{name}(bool on) +// accessors on the QuicSession class. +#define QUICSESSION_FLAGS(V) \ + V(CLOSING, closing) \ + V(GRACEFUL_CLOSING, graceful_closing) \ + V(DESTROYED, destroyed) \ + V(TRANSPORT_PARAMS_SET, transport_params_set) \ + V(NGTCP2_CALLBACK, in_ngtcp2_callback) \ + V(SILENT_CLOSE, silent_closing) \ + V(STATELESS_RESET, stateless_reset) + // The QuicSession class is an virtual class that serves as // the basis for both client and server QuicSession. // It implements the functionality that is shared for both @@ -801,15 +835,14 @@ class QuicSession : public AsyncWrap, inline bool allow_early_data() const; - // Returns true if StartGracefulClose() has been called and the - // QuicSession is currently in the process of a graceful close. - inline bool is_gracefully_closing() const; - - // Returns true if Destroy() has been called and the - // QuicSession is no longer usable. - inline bool is_destroyed() const; - - inline bool is_stateless_reset() const; +#define V(id, name) \ + bool is_##name() const { return flags_ & (1 << QUICSESSION_FLAG_##id); } \ + void set_##name(bool on = true) { \ + if (on) flags_ |= (1 << QUICSESSION_FLAG_##id); \ + else flags_ &= ~(1 << QUICSESSION_FLAG_##id); \ + } + QUICSESSION_FLAGS(V) +#undef V // Returns true if the QuicSession has entered the // closing period following a call to ImmediateClose. @@ -1089,15 +1122,15 @@ class QuicSession : public AsyncWrap, explicit Ngtcp2CallbackScope(QuicSession* session) : session_(session) { CHECK(session_); CHECK(!InNgtcp2CallbackScope(session)); - session_->set_flag(QUICSESSION_FLAG_NGTCP2_CALLBACK); + session_->set_in_ngtcp2_callback(); } ~Ngtcp2CallbackScope() { - session_->set_flag(QUICSESSION_FLAG_NGTCP2_CALLBACK, false); + session_->set_in_ngtcp2_callback(false); } static bool InNgtcp2CallbackScope(QuicSession* session) { - return session->is_flag_set(QUICSESSION_FLAG_NGTCP2_CALLBACK); + return session->is_in_ngtcp2_callback(); } private: @@ -1356,55 +1389,12 @@ class QuicSession : public AsyncWrap, bool StartClosingPeriod(); +#define V(id, _) QUICSESSION_FLAG_##id, enum QuicSessionFlags : uint32_t { - // Initial state when a QuicSession is created but nothing yet done. - QUICSESSION_FLAG_INITIAL = 0x1, - - // Set while the QuicSession is in the process of an Immediate - // or silent close. - QUICSESSION_FLAG_CLOSING = 0x2, - - // Set while the QuicSession is in the process of a graceful close. - QUICSESSION_FLAG_GRACEFUL_CLOSING = 0x4, - - // Set when the QuicSession has been destroyed (but not - // yet freed) - QUICSESSION_FLAG_DESTROYED = 0x8, - - QUICSESSION_FLAG_HAS_TRANSPORT_PARAMS = 0x10, - - // Set while the QuicSession is executing an ngtcp2 callback - QUICSESSION_FLAG_NGTCP2_CALLBACK = 0x100, - - // Set if the QuicSession is in the middle of a silent close - // (that is, a CONNECTION_CLOSE should not be sent) - QUICSESSION_FLAG_SILENT_CLOSE = 0x200, - - QUICSESSION_FLAG_HANDSHAKE_RX = 0x400, - QUICSESSION_FLAG_HANDSHAKE_TX = 0x800, - QUICSESSION_FLAG_HANDSHAKE_KEYS = - QUICSESSION_FLAG_HANDSHAKE_RX | - QUICSESSION_FLAG_HANDSHAKE_TX, - QUICSESSION_FLAG_SESSION_RX = 0x1000, - QUICSESSION_FLAG_SESSION_TX = 0x2000, - QUICSESSION_FLAG_SESSION_KEYS = - QUICSESSION_FLAG_SESSION_RX | - QUICSESSION_FLAG_SESSION_TX, - - // Set if the QuicSession was closed due to stateless reset - QUICSESSION_FLAG_STATELESS_RESET = 0x4000 + QUICSESSION_FLAGS(V) + QUICSESSION_FLAG_COUNT }; - - void set_flag(QuicSessionFlags flag, bool on = true) { - if (on) - flags_ |= flag; - else - flags_ &= ~flag; - } - - bool is_flag_set(QuicSessionFlags flag) const { - return (flags_ & flag) == flag; - } +#undef V void IncrementConnectionCloseAttempts() { if (connection_close_attempts_ < kMaxSizeT) diff --git a/src/quic/node_quic_socket-inl.h b/src/quic/node_quic_socket-inl.h index 38f3ad927180f0..bcc85d82314f22 100644 --- a/src/quic/node_quic_socket-inl.h +++ b/src/quic/node_quic_socket-inl.h @@ -96,9 +96,9 @@ void QuicSocket::DisassociateStatelessResetToken( // existing sessions are allowed to close naturally but new // sessions are rejected. void QuicSocket::StopListening() { - if (is_flag_set(QUICSOCKET_FLAGS_SERVER_LISTENING)) { + if (is_server_listening()) { Debug(this, "Stop listening"); - set_flag(QUICSOCKET_FLAGS_SERVER_LISTENING, false); + set_server_listening(false); // It is important to not call ReceiveStop here as there // is ongoing traffic being exchanged by the peers. } @@ -155,9 +155,9 @@ size_t QuicSocket::GetCurrentStatelessResetCounter(const SocketAddress& addr) { return it == std::end(reset_counts_) ? 0 : it->second; } -void QuicSocket::set_server_busy(bool on) { +void QuicSocket::ServerBusy(bool on) { Debug(this, "Turning Server Busy Response %s", on ? "on" : "off"); - set_flag(QUICSOCKET_FLAGS_SERVER_BUSY, on); + set_server_busy(on); listener_->OnServerBusy(on); } @@ -174,14 +174,12 @@ void QuicSocket::set_diagnostic_packet_loss(double rx, double tx) { } bool QuicSocket::ToggleStatelessReset() { - set_flag( - QUICSOCKET_FLAGS_DISABLE_STATELESS_RESET, - !is_flag_set(QUICSOCKET_FLAGS_DISABLE_STATELESS_RESET)); - return !is_flag_set(QUICSOCKET_FLAGS_DISABLE_STATELESS_RESET); + set_stateless_reset_disabled(!is_stateless_reset_disabled()); + return !is_stateless_reset_disabled(); } void QuicSocket::set_validated_address(const SocketAddress& addr) { - if (is_option_set(QUICSOCKET_OPTIONS_VALIDATE_ADDRESS_LRU)) { + if (has_option_validate_address_lru()) { // Remove the oldest item if we've hit the LRU limit validated_addrs_.push_back(SocketAddress::Hash()(addr)); if (validated_addrs_.size() > kMaxValidateAddressLru) @@ -190,7 +188,7 @@ void QuicSocket::set_validated_address(const SocketAddress& addr) { } bool QuicSocket::is_validated_address(const SocketAddress& addr) const { - if (is_option_set(QUICSOCKET_OPTIONS_VALIDATE_ADDRESS_LRU)) { + if (has_option_validate_address_lru()) { auto res = std::find(std::begin(validated_addrs_), std::end(validated_addrs_), SocketAddress::Hash()(addr)); @@ -217,7 +215,7 @@ void QuicSocket::AddEndpoint( if (preferred || endpoints_.empty()) preferred_endpoint_ = endpoint_; endpoints_.emplace_back(endpoint_); - if (is_flag_set(QUICSOCKET_FLAGS_SERVER_LISTENING)) + if (is_server_listening()) endpoint_->ReceiveStart(); } diff --git a/src/quic/node_quic_socket.cc b/src/quic/node_quic_socket.cc index e3112887d8e4fb..6ebdb30ef8a250 100644 --- a/src/quic/node_quic_socket.cc +++ b/src/quic/node_quic_socket.cc @@ -267,7 +267,7 @@ QuicSocket::QuicSocket( EntropySource(token_secret_, kTokenSecretLen); if (disable_stateless_reset) - set_flag(QUICSOCKET_FLAGS_DISABLE_STATELESS_RESET); + set_stateless_reset_disabled(); // Set the session reset secret to the one provided or random. // Note that a random secret is going to make it exceedingly @@ -316,13 +316,13 @@ void QuicSocket::Listen( const std::string& alpn, uint32_t options) { CHECK(sc); - CHECK(!is_flag_set(QUICSOCKET_FLAGS_SERVER_LISTENING)); + CHECK(!is_server_listening()); Debug(this, "Starting to listen"); server_session_config_.Set(quic_state(), preferred_address); server_secure_context_ = sc; server_alpn_ = alpn; server_options_ = options; - set_flag(QUICSOCKET_FLAGS_SERVER_LISTENING); + set_server_listening(); RecordTimestamp(&QuicSocketStats::listen_at); ReceiveStart(); } @@ -737,7 +737,7 @@ BaseObjectPtr QuicSocket::AcceptInitialPacket( QuicCID ocid; // If the QuicSocket is not listening, the paket will be ignored. - if (!is_flag_set(QUICSOCKET_FLAGS_SERVER_LISTENING)) { + if (!is_server_listening()) { Debug(this, "QuicSocket is not listening"); return {}; } @@ -762,7 +762,7 @@ BaseObjectPtr QuicSocket::AcceptInitialPacket( // Else, check to see if the number of connections total for this QuicSocket // has been exceeded. If the count has been exceeded, shutdown the connection // immediately after the initial keys are installed. - if (UNLIKELY(is_flag_set(QUICSOCKET_FLAGS_SERVER_BUSY)) || + if (UNLIKELY(is_server_busy()) || sessions_.size() >= max_connections_ || GetCurrentSocketAddressCounter(remote_addr) >= max_connections_per_host_) { @@ -786,8 +786,7 @@ BaseObjectPtr QuicSocket::AcceptInitialPacket( if (!is_validated_address(remote_addr)) { switch (hd.type) { case NGTCP2_PKT_INITIAL: - if (is_option_set(QUICSOCKET_OPTIONS_VALIDATE_ADDRESS) || - hd.token.len > 0) { + if (has_option_validate_address() || hd.token.len > 0) { Debug(this, "Performing explicit address validation"); if (hd.token.len == 0) { Debug(this, "No retry token was detected. Generating one"); @@ -1124,7 +1123,7 @@ void QuicSocketSetServerBusy(const FunctionCallbackInfo& args) { QuicSocket* socket; ASSIGN_OR_RETURN_UNWRAP(&socket, args.Holder()); CHECK_EQ(args.Length(), 1); - socket->set_server_busy(args[0]->IsTrue()); + socket->ServerBusy(args[0]->IsTrue()); } void QuicSocketToggleStatelessReset(const FunctionCallbackInfo& args) { diff --git a/src/quic/node_quic_socket.h b/src/quic/node_quic_socket.h index 2fcf0fb7f19d31..a9d4058fb7a1e0 100644 --- a/src/quic/node_quic_socket.h +++ b/src/quic/node_quic_socket.h @@ -36,17 +36,23 @@ namespace quic { class QuicSocket; class QuicEndpoint; +#define QUICSOCKET_OPTIONS(V) \ + V(VALIDATE_ADDRESS, validate_address) \ + V(VALIDATE_ADDRESS_LRU, validate_address_lru) + +#define V(id, _) QUICSOCKET_OPTIONS_##id, enum QuicSocketOptions : uint32_t { - // When enabled the QuicSocket will validate the address - // using a RETRY packet to the peer. - QUICSOCKET_OPTIONS_VALIDATE_ADDRESS = 0x1, - - // When enabled, and the VALIDATE_ADDRESS option is also - // set, the QuicSocket will use an LRU cache to track - // validated addresses. Address validation will be skipped - // if the address is currently in the cache. - QUICSOCKET_OPTIONS_VALIDATE_ADDRESS_LRU = 0x2, + QUICSOCKET_OPTIONS(V) + QUICSOCKET_OPTIONS_COUNT }; +#undef V + +#define QUICSOCKET_FLAGS(V) \ + V(GRACEFUL_CLOSE, graceful_closing) \ + V(WAITING_FOR_CALLBACKS, waiting_for_callbacks) \ + V(SERVER_LISTENING, server_listening) \ + V(SERVER_BUSY, server_busy) \ + V(DISABLE_STATELESS_RESET, stateless_reset_disabled) #define SOCKET_STATS(V) \ V(CREATED_AT, created_at, "Created At") \ @@ -351,7 +357,25 @@ class QuicSocket : public AsyncWrap, std::unique_ptr packet, BaseObjectPtr session = BaseObjectPtr()); - inline void set_server_busy(bool on); +#define V(id, name) \ + inline bool is_##name() const { \ + return flags_ & (1 << QUICSOCKET_FLAG_##id); } \ + inline void set_##name(bool on = true) { \ + if (on) \ + flags_ |= (1 << QUICSOCKET_FLAG_##id); \ + else \ + flags_ &= ~(1 << QUICSOCKET_FLAG_##id); \ + } + QUICSOCKET_FLAGS(V) +#undef V + +#define V(id, name) \ + bool has_option_##name() const { \ + return options_ & (1 << QUICSOCKET_OPTIONS_##id); } + QUICSOCKET_OPTIONS(V) +#undef V + + inline void ServerBusy(bool on); inline void set_diagnostic_packet_loss(double rx = 0.0, double tx = 0.0); @@ -489,43 +513,12 @@ class QuicSocket : public AsyncWrap, // and the current packet should be artificially considered lost. inline bool is_diagnostic_packet_loss(double prob) const; - bool is_stateless_reset_disabled() { - return is_flag_set(QUICSOCKET_FLAGS_DISABLE_STATELESS_RESET); - } - +#define V(id, _) QUICSOCKET_FLAG_##id, enum QuicSocketFlags : uint32_t { - QUICSOCKET_FLAGS_NONE = 0x0, - - // Indicates that the QuicSocket has entered a graceful - // closing phase, indicating that no additional - QUICSOCKET_FLAGS_GRACEFUL_CLOSE = 0x1, - QUICSOCKET_FLAGS_WAITING_FOR_CALLBACKS = 0x2, - QUICSOCKET_FLAGS_SERVER_LISTENING = 0x4, - QUICSOCKET_FLAGS_SERVER_BUSY = 0x8, - QUICSOCKET_FLAGS_DISABLE_STATELESS_RESET = 0x10 + QUICSOCKET_FLAGS(V) + QUICSOCKET_FLAG_COUNT }; - - void set_flag(QuicSocketFlags flag, bool on = true) { - if (on) - flags_ |= flag; - else - flags_ &= ~flag; - } - - bool is_flag_set(QuicSocketFlags flag) const { - return flags_ & flag; - } - - void set_option(QuicSocketOptions option, bool on = true) { - if (on) - options_ |= option; - else - options_ &= ~option; - } - - bool is_option_set(QuicSocketOptions option) const { - return options_ & option; - } +#undef V ngtcp2_mem alloc_info_; @@ -533,8 +526,8 @@ class QuicSocket : public AsyncWrap, SocketAddress::Map> bound_endpoints_; BaseObjectWeakPtr preferred_endpoint_; - uint32_t flags_ = QUICSOCKET_FLAGS_NONE; - uint32_t options_; + uint32_t flags_ = 0; + uint32_t options_ = 0; uint32_t server_options_; size_t max_connections_ = DEFAULT_MAX_CONNECTIONS; diff --git a/src/quic/node_quic_stream-inl.h b/src/quic/node_quic_stream-inl.h index 546e867ee2db7e..e253875c4d3389 100644 --- a/src/quic/node_quic_stream-inl.h +++ b/src/quic/node_quic_stream-inl.h @@ -23,31 +23,16 @@ QuicStreamOrigin QuicStream::origin() const { QUIC_STREAM_CLIENT; } -bool QuicStream::is_flag_set(int32_t flag) const { - return flags_ & (1 << flag); -} - -void QuicStream::set_flag(int32_t flag, bool on) { - if (on) - flags_ |= (1 << flag); - else - flags_ &= ~(1 << flag); -} - void QuicStream::set_final_size(uint64_t final_size) { // Only set the final size once. - if (is_flag_set(QUICSTREAM_FLAG_FIN)) { + if (is_fin()) { CHECK_LE(final_size, GetStat(&QuicStreamStats::final_size)); return; } - set_flag(QUICSTREAM_FLAG_FIN); + set_fin(true); SetStat(&QuicStreamStats::final_size, final_size); } -bool QuicStream::is_destroyed() const { - return is_flag_set(QUICSTREAM_FLAG_DESTROYED); -} - bool QuicStream::was_ever_writable() const { if (direction() == QUIC_STREAM_UNIDIRECTIONAL) { return session_->is_server() ? @@ -72,17 +57,11 @@ bool QuicStream::was_ever_readable() const { } bool QuicStream::is_readable() const { - return was_ever_readable() && !is_flag_set(QUICSTREAM_FLAG_READ_CLOSED); -} - -void QuicStream::set_fin_sent() { - CHECK(!is_writable()); - set_flag(QUICSTREAM_FLAG_FIN_SENT); + return was_ever_readable() && !is_read_closed(); } bool QuicStream::is_write_finished() const { - return is_flag_set(QUICSTREAM_FLAG_FIN_SENT) && - streambuf_.length() == 0; + return is_fin_sent() && streambuf_.length() == 0; } bool QuicStream::SubmitInformation(v8::Local headers) { @@ -143,7 +122,7 @@ void QuicStream::ResetStream(uint64_t app_error_code) { session()->connection(), stream_id_, app_error_code); - set_flag(QUICSTREAM_FLAG_READ_CLOSED); + set_read_closed(); streambuf_.Cancel(); streambuf_.End(); } @@ -156,7 +135,7 @@ void QuicStream::StopSending(uint64_t app_error_code) { session()->connection(), stream_id_, app_error_code); - set_flag(QUICSTREAM_FLAG_READ_CLOSED); + set_read_closed(); } void QuicStream::Schedule(Queue* queue) { diff --git a/src/quic/node_quic_stream.cc b/src/quic/node_quic_stream.cc index ba447e760d206a..3b4e7286945957 100644 --- a/src/quic/node_quic_stream.cc +++ b/src/quic/node_quic_stream.cc @@ -124,8 +124,8 @@ void QuicStream::Destroy(QuicError* error) { QuicSession::SendSessionScope send_scope(session()); - set_flag(QUICSTREAM_FLAG_DESTROYED); - set_flag(QUICSTREAM_FLAG_READ_CLOSED); + set_read_closed(); + set_destroyed(); // In case this stream is scheduled for sending, remove it // from the schedule queue @@ -234,8 +234,8 @@ bool QuicStream::IsClosing() { int QuicStream::ReadStart() { CHECK(!is_destroyed()); CHECK(is_readable()); - set_flag(QUICSTREAM_FLAG_READ_STARTED); - set_flag(QUICSTREAM_FLAG_READ_PAUSED, false); + set_read_started(); + set_read_paused(false); IncrementStat( &QuicStreamStats::max_offset, inbound_consumed_data_while_paused_); @@ -246,7 +246,7 @@ int QuicStream::ReadStart() { int QuicStream::ReadStop() { CHECK(!is_destroyed()); CHECK(is_readable()); - set_flag(QUICSTREAM_FLAG_READ_PAUSED); + set_read_paused(); return 0; } @@ -348,7 +348,7 @@ void QuicStream::ReceiveData( datalen -= avail; // Capture read_paused before EmitRead in case user code callbacks // alter the state when EmitRead is called. - bool read_paused = is_flag_set(QUICSTREAM_FLAG_READ_PAUSED); + bool read_paused = is_read_paused(); EmitRead(avail, buf); // Reading can be paused while we are processing. If that's // the case, we still want to acknowledge the current bytes diff --git a/src/quic/node_quic_stream.h b/src/quic/node_quic_stream.h index 0d0809c74df0ed..53a8cdaea52d51 100644 --- a/src/quic/node_quic_stream.h +++ b/src/quic/node_quic_stream.h @@ -78,30 +78,13 @@ struct QuicStreamStatsTraits { static void ToString(const Base& ptr, Fn&& add_field); }; -enum QuicStreamStates : uint32_t { - // QuicStream is fully open. Readable and Writable - QUICSTREAM_FLAG_INITIAL = 0, - - // QuicStream Read State is closed because a final stream frame - // has been received from the peer or the QuicStream is unidirectional - // outbound only (i.e. it was never readable) - QUICSTREAM_FLAG_READ_CLOSED, - - // JavaScript side has switched into flowing mode (Readable side) - QUICSTREAM_FLAG_READ_STARTED, - - // JavaScript side has paused the flow of data (Readable side) - QUICSTREAM_FLAG_READ_PAUSED, - - // QuicStream has received a final stream frame (Readable side) - QUICSTREAM_FLAG_FIN, - - // QuicStream has sent a final stream frame (Writable side) - QUICSTREAM_FLAG_FIN_SENT, - - // QuicStream has been destroyed - QUICSTREAM_FLAG_DESTROYED -}; +#define QUICSTREAM_FLAGS(V) \ + V(READ_CLOSED, read_closed) \ + V(READ_STARTED, read_started) \ + V(READ_PAUSED, read_paused) \ + V(FIN, fin) \ + V(FIN_SENT, fin_sent) \ + V(DESTROYED, destroyed) enum QuicStreamDirection { // The QuicStream is readable and writable in both directions @@ -228,9 +211,6 @@ class QuicStream : public AsyncWrap, // or the server. inline QuicStreamOrigin origin() const; - // The QuicStream has been destroyed and is no longer usable. - inline bool is_destroyed() const; - // A QuicStream will not be writable if: // - The streambuf_ is ended // - It is a Unidirectional stream originating from the peer @@ -241,13 +221,6 @@ class QuicStream : public AsyncWrap, // - It is a Unidirectional stream originating from the local peer. inline bool is_readable() const; - // Records the fact that a final stream frame has been - // serialized and sent to the peer. There still may be - // unacknowledged data in the outbound queue, but no - // additional frames may be sent for the stream other - // than reset stream. - inline void set_fin_sent(); - // IsWriteFinished will return true if a final stream frame // has been sent and all data has been acknowledged (the // send buffer is empty). @@ -342,6 +315,18 @@ class QuicStream : public AsyncWrap, // Required for StreamBase int ReadStop() override; +#define V(id, name) \ + inline bool is_##name() const { \ + return flags_ & (1 << QUICSTREAM_FLAG_##id); } \ + inline void set_##name(bool on = true) { \ + if (on) \ + flags_ |= (1 << QUICSTREAM_FLAG_##id); \ + else \ + flags_ &= ~(1 << QUICSTREAM_FLAG_##id); \ + } + QUICSTREAM_FLAGS(V) +#undef V + // Required for StreamBase int DoShutdown(ShutdownWrap* req_wrap) override; @@ -363,10 +348,6 @@ class QuicStream : public AsyncWrap, size_t max_count_hint) override; private: - inline bool is_flag_set(int32_t flag) const; - - inline void set_flag(int32_t flag, bool on = true); - // WasEverWritable returns true if it is a bidirectional stream, // or a Unidirectional stream originating from the local peer. // If was_ever_writable() is false, then no stream frames should @@ -380,12 +361,20 @@ class QuicStream : public AsyncWrap, void IncrementStats(size_t datalen); +#define V(id, _) QUICSTREAM_FLAG_##id, + enum QuicStreamStates : uint32_t { + QUICSTREAM_FLAGS(V) + QUICSTREAM_FLAG_COUNT + }; +#undef V + BaseObjectWeakPtr session_; QuicBuffer streambuf_; int64_t stream_id_ = 0; int64_t push_id_ = 0; - uint32_t flags_ = QUICSTREAM_FLAG_INITIAL; + uint32_t flags_ = 0; + size_t inbound_consumed_data_while_paused_ = 0; std::vector> headers_; From f7510ca439eb17061d075cc73bef640d2ef104a4 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 1 Jul 2020 09:39:22 -0700 Subject: [PATCH 03/25] quic: additional cleanups on the c++ side PR-URL: https://github.com/nodejs/node/pull/34160 Reviewed-By: Anna Henningsen --- src/quic/node_quic_http3_application.cc | 4 +- src/quic/node_quic_session.cc | 74 +++++++++++++------------ src/quic/node_quic_session.h | 51 ++++++----------- 3 files changed, 58 insertions(+), 71 deletions(-) diff --git a/src/quic/node_quic_http3_application.cc b/src/quic/node_quic_http3_application.cc index 7dc41a392e54df..150734bb81c925 100644 --- a/src/quic/node_quic_http3_application.cc +++ b/src/quic/node_quic_http3_application.cc @@ -594,8 +594,8 @@ void Http3Application::StreamClosed( int64_t stream_id, uint64_t app_error_code) { BaseObjectPtr stream = session()->FindStream(stream_id); - CHECK(stream); - stream->ReceiveData(1, nullptr, 0, 0); + if (stream) + stream->ReceiveData(1, nullptr, 0, 0); session()->listener()->OnStreamClose(stream_id, app_error_code); } diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index d28f97cdba8fb8..5f102eb2f9362f 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -757,6 +757,10 @@ void QuicSession::RandomConnectionIDStrategy( # define HAVE_SSL_TRACE 1 #endif +QuicCryptoContext::~QuicCryptoContext() { + // Free any remaining crypto handshake data (if any) + Cancel(); +} void QuicCryptoContext::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("initial_crypto", handshake_[0]); @@ -1470,8 +1474,6 @@ QuicSession::QuicSession( QuicSession::~QuicSession() { CHECK(!Ngtcp2CallbackScope::InNgtcp2CallbackScope(this)); - crypto_context_->Cancel(); - connection_.reset(); QuicSessionListener* listener_ = listener(); listener_->OnSessionDestroyed(); @@ -1637,7 +1639,9 @@ void QuicSession::AddStream(BaseObjectPtr stream) { // not immediately torn down, but is allowed to drain // properly per the QUIC spec description of "immediate close". void QuicSession::ImmediateClose() { - if (is_closing() || is_silent_closing()) + // If ImmediateClose or SilentClose has already been called, + // do not re-enter. + if (is_closing()) return; set_closing(); @@ -1649,6 +1653,35 @@ void QuicSession::ImmediateClose() { listener()->OnSessionClose(err); } +// Silent Close must start with the JavaScript side, which must +// clean up state, abort any still existing QuicSessions, then +// destroy the handle when done. The most important characteristic +// of the SilentClose is that no frames are sent to the peer. +// +// When a valid stateless reset is received, the connection is +// immediately and unrecoverably closed at the ngtcp2 level. +// Specifically, it will be put into the draining_period so +// absolutely no frames can be sent. What we need to do is +// notify the JavaScript side and destroy the connection with +// a flag set that indicates stateless reset. +void QuicSession::SilentClose() { + CHECK(!is_silent_closing()); + set_silent_closing(); + set_closing(); + + QuicError err = last_error(); + Debug(this, + "Silent close with %s code %" PRIu64 " (stateless reset? %s)", + err.family_name(), + err.code, + is_stateless_reset() ? "yes" : "no"); + + int flags = QuicSessionListener::SESSION_CLOSE_FLAG_SILENT; + if (is_stateless_reset()) + flags |= QuicSessionListener::SESSION_CLOSE_FLAG_STATELESS_RESET; + listener()->OnSessionClose(err, flags); +} + // Creates a new stream object and passes it off to the javascript side. // This has to be called from within a handlescope/contextscope. BaseObjectPtr QuicSession::CreateStream(int64_t stream_id) { @@ -1958,7 +1991,7 @@ bool QuicSession::ReceivePacket( // then immediately close the connection. if (err == NGTCP2_ERR_RETRY && is_server()) { socket()->SendRetry(scid_, dcid_, local_address_, remote_address_); - ImmediateClose(); + SilentClose(); break; } set_last_error(QUIC_ERROR_SESSION, err); @@ -2050,7 +2083,7 @@ void QuicSession::RemoveFromSocket() { void QuicSession::RemoveStream(int64_t stream_id) { Debug(this, "Removing stream %" PRId64, stream_id); - // ngtcp2 does no extend the max streams count automatically + // ngtcp2 does not extend the max streams count automatically // except in very specific conditions, none of which apply // once we've gotten this far. We need to manually extend when // a remote peer initiated stream is removed. @@ -2104,6 +2137,8 @@ bool QuicSession::SendConnectionClose() { // it multiple times; whereas for clients, we will serialize it // once and send once only. QuicError error = last_error(); + Debug(this, "Connection Close code: %" PRIu64 " (family: %s)", + error.code, error.family_name()); // If initial keys have not yet been installed, use the alternative // ImmediateConnectionClose to send a stateless connection close to @@ -2322,34 +2357,6 @@ void QuicSession::ResumeStream(int64_t stream_id) { application()->ResumeStream(stream_id); } -// Silent Close must start with the JavaScript side, which must -// clean up state, abort any still existing QuicSessions, then -// destroy the handle when done. The most important characteristic -// of the SilentClose is that no frames are sent to the peer. -// -// When a valid stateless reset is received, the connection is -// immediately and unrecoverably closed at the ngtcp2 level. -// Specifically, it will be put into the draining_period so -// absolutely no frames can be sent. What we need to do is -// notify the JavaScript side and destroy the connection with -// a flag set that indicates stateless reset. -void QuicSession::SilentClose() { - CHECK(!is_silent_closing()); - set_silent_closing(); - set_closing(); - - QuicError err = last_error(); - Debug(this, - "Silent close with %s code %" PRIu64 " (stateless reset? %s)", - err.family_name(), - err.code, - is_stateless_reset() ? "yes" : "no"); - - int flags = QuicSessionListener::SESSION_CLOSE_FLAG_SILENT; - if (is_stateless_reset()) - flags |= QuicSessionListener::SESSION_CLOSE_FLAG_STATELESS_RESET; - listener()->OnSessionClose(err, flags); -} // Begin connection close by serializing the CONNECTION_CLOSE packet. // There are two variants: one to serialize an application close, the // other to serialize a protocol close. The frames are generally @@ -2508,7 +2515,6 @@ void QuicSession::UpdateIdleTimer() { // serialize stream data that is being sent initially. bool QuicSession::WritePackets(const char* diagnostic_label) { CHECK(!Ngtcp2CallbackScope::InNgtcp2CallbackScope(this)); - CHECK(!is_destroyed()); // During the draining period, we must not send any frames at all. if (is_in_draining_period()) diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index 8c22fe3bdab534..3087ecf12c947b 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -369,6 +369,14 @@ class JSQuicSessionListener : public QuicSessionListener { // handshake details on behalf of a QuicSession. class QuicCryptoContext : public MemoryRetainer { public: + inline QuicCryptoContext( + QuicSession* session, + BaseObjectPtr secure_context, + ngtcp2_crypto_side side, + uint32_t options); + + ~QuicCryptoContext() override; + inline uint64_t Cancel(); // Outgoing crypto data must be retained in memory until it is @@ -482,12 +490,6 @@ class QuicCryptoContext : public MemoryRetainer { SET_SELF_SIZE(QuicCryptoContext) private: - inline QuicCryptoContext( - QuicSession* session, - BaseObjectPtr secure_context, - ngtcp2_crypto_side side, - uint32_t options); - bool SetSecrets( ngtcp2_crypto_level level, const uint8_t* rx_secret, @@ -1038,37 +1040,16 @@ class QuicSession : public AsyncWrap, inline void DecreaseAllocatedSize(size_t size); - // Immediately close the QuicSession. All currently open - // streams are implicitly reset and closed with RESET_STREAM - // and STOP_SENDING frames transmitted as necessary. A - // CONNECTION_CLOSE frame will be sent and the session - // will enter the closing period until either the idle - // timeout period elapses or until the QuicSession is - // explicitly destroyed. During the closing period, - // the only frames that may be transmitted to the peer - // are repeats of the already sent CONNECTION_CLOSE. - // - // The CONNECTION_CLOSE will use the error code set using - // the most recent call to set_last_error() + // Triggers an "immediate close" on the QuicSession. + // This will round trip through JavaScript, causing + // all currently open streams to be closed and ultimately + // send a CONNECTION_CLOSE to the connected peer before + // terminating the connection. void ImmediateClose(); - // Silently, and immediately close the QuicSession. This is - // generally only done during an idle timeout. That is, per - // the QUIC specification, if the session remains idle for - // longer than both the advertised idle timeout and three - // times the current probe timeout (PTO). In such cases, all - // currently open streams are implicitly reset and closed - // without sending corresponding RESET_STREAM and - // STOP_SENDING frames, the connection state is - // discarded, and the QuicSession is destroyed without - // sending a CONNECTION_CLOSE frame. - // - // Silent close may also be used to explicitly destroy - // a QuicSession that has either already entered the - // closing or draining periods; or in response to user - // code requests to forcefully terminate a QuicSession - // without transmitting any additional frames to the - // peer. + // Silently and immediately close the QuicSession. This is + // typically only done during an idle timeout or when sending + // a retry packet. void SilentClose(); void PushListener(QuicSessionListener* listener); From f9c2245fb5122e9b617947a5b3f5623e44df88e4 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 1 Jul 2020 15:06:27 -0700 Subject: [PATCH 04/25] quic: refactor QuicSession close/destroy flow PR-URL: https://github.com/nodejs/node/pull/34160 Reviewed-By: Anna Henningsen --- lib/internal/quic/core.js | 121 ++++++------------ src/quic/node_quic_session-inl.h | 2 +- src/quic/node_quic_session.cc | 211 ++++++++++++------------------- src/quic/node_quic_session.h | 67 +++++++--- src/quic/node_quic_socket.cc | 2 + 5 files changed, 174 insertions(+), 229 deletions(-) diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index 90727194246450..75f07c8d7e5871 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -287,24 +287,10 @@ function onSessionReady(handle) { process.nextTick(emit.bind(socket, 'session', session)); } -// Called when the session needs to be closed and destroyed. -// If silent is true, then the session is going to be closed -// immediately without sending any CONNECTION_CLOSE to the -// connected peer. If silent is false, a CONNECTION_CLOSE -// is going to be sent to the peer. +// Called when the C++ QuicSession::Close() method has been called. +// Synchronously cleanup and destroy the JavaScript QuicSession. function onSessionClose(code, family, silent, statelessReset) { - if (this[owner_symbol]) { - if (silent) { - this[owner_symbol][kDestroy](statelessReset, family, code); - } else { - this[owner_symbol][kClose](family, code); - } - return; - } - // When there's no owner_symbol, the session was closed - // before it could be fully set up. Just immediately - // close everything down on the native side. - this.destroy(code, family); + this[owner_symbol][kDestroy](code, family, silent, statelessReset); } // Called by the C++ internals when a QuicSession has been destroyed. @@ -1654,6 +1640,7 @@ class QuicSession extends EventEmitter { maxPacketLength: NGTCP2_DEFAULT_MAX_PKTLEN, servername: undefined, socket: undefined, + silentClose: false, statelessReset: false, stats: undefined, pendingStreams: new Set(), @@ -1736,46 +1723,15 @@ class QuicSession extends EventEmitter { // Causes the QuicSession to be immediately destroyed, but with // additional metadata set. - [kDestroy](statelessReset, family, code) { + [kDestroy](code, family, silent, statelessReset) { const state = this[kInternalState]; - state.statelessReset = statelessReset; state.closeCode = code; state.closeFamily = family; + state.silentClose = silent; + state.statelessReset = statelessReset; this.destroy(); } - // Immediate close has been initiated for the session. Any - // still open QuicStreams must be abandoned and shutdown - // with RESET_STREAM and STOP_SENDING frames transmitted - // as appropriate. Once the streams have been shutdown, a - // CONNECTION_CLOSE will be generated and sent, switching - // the session into the closing period. - [kClose](family, code) { - const state = this[kInternalState]; - // Do nothing if the QuicSession has already been destroyed. - if (state.destroyed) - return; - - // Set the close code and family so we can keep track. - state.closeCode = code; - state.closeFamily = family; - - // Shutdown all pending streams. These are Streams that - // have been created but do not yet have a handle assigned. - for (const stream of state.pendingStreams) - stream[kClose](family, code); - - // Shutdown all of the remaining streams - for (const stream of state.streams.values()) - stream[kClose](family, code); - - // By this point, all necessary RESET_STREAM and - // STOP_SENDING frames ought to have been sent, - // so now we just trigger sending of the - // CONNECTION_CLOSE frame. - this[kHandle].close(family, code); - } - // Closes the specified stream with the given code. The // QuicStream object will be destroyed. [kStreamClose](id, code) { @@ -1873,14 +1829,6 @@ class QuicSession extends EventEmitter { this[kInternalState].streams.set(id, stream); } - // The QuicSession will be destroyed if closing has been - // called and there are no remaining streams - [kMaybeDestroy]() { - const state = this[kInternalState]; - if (state.closing && state.streams.size === 0) - this.destroy(); - } - // Called when a client QuicSession has opted to use the // server provided preferred address. This is a purely // informationational notification. It is not called on @@ -1890,6 +1838,17 @@ class QuicSession extends EventEmitter { emit.bind(this, 'usePreferredAddress', address)); } + // The QuicSession will be destroyed if close() has been + // called and there are no remaining streams + [kMaybeDestroy]() { + const state = this[kInternalState]; + if (state.closing && state.streams.size === 0) { + this.destroy(); + return true; + } + return false; + } + // Closing allows any existing QuicStream's to complete // normally but disallows any new QuicStreams from being // opened. Calls to openStream() will fail, and new streams @@ -1910,27 +1869,27 @@ class QuicSession extends EventEmitter { // has been destroyed if (state.closing) return; - state.closing = true; - this[kHandle].gracefulClose(); - // See if we can close immediately. - this[kMaybeDestroy](); + // If there are no active streams, we can close immediately, + // otherwise set the graceful close flag. + if (!this[kMaybeDestroy]()) + this[kHandle].gracefulClose(); } // Destroying synchronously shuts down and frees the // QuicSession immediately, even if there are still open // streams. // - // A CONNECTION_CLOSE packet is sent to the - // connected peer and the session is immediately - // destroyed. + // Unless we're in the middle of a silent close, a + // CONNECTION_CLOSE packet will be sent to the connected + // peer and the session will be immediately destroyed. // // If destroy is called with an error argument, the // 'error' event is emitted on next tick. // // Once destroyed, and after the 'error' event (if any), - // the close event is emitted on next tick. + // the 'close' event is emitted on next tick. destroy(error) { const state = this[kInternalState]; // Destroy can only be called once. Multiple calls will be ignored @@ -1939,19 +1898,6 @@ class QuicSession extends EventEmitter { state.destroyed = true; state.closing = false; - if (typeof error === 'number' || - (error != null && - typeof error === 'object' && - !(error instanceof Error))) { - const { - closeCode, - closeFamily - } = validateCloseCode(error); - state.closeCode = closeCode; - state.closeFamily = closeFamily; - error = new ERR_QUIC_ERROR(closeCode, closeFamily); - } - // Destroy any pending streams immediately. These // are streams that have been created but have not // yet been assigned an internal handle. @@ -1965,16 +1911,20 @@ class QuicSession extends EventEmitter { this.removeListener('newListener', onNewListener); this.removeListener('removeListener', onRemoveListener); + // If we are destroying with an error, schedule the + // error to be emitted on process.nextTick. if (error) process.nextTick(emit.bind(this, 'error', error)); const handle = this[kHandle]; + this[kHandle] = undefined; + if (handle !== undefined) { // Copy the stats for use after destruction state.stats = new BigInt64Array(handle.stats); - state.idleTimeout = !!handle.state[IDX_QUIC_SESSION_STATE_IDLE_TIMEOUT]; - // Calling destroy will cause a CONNECTION_CLOSE to be - // sent to the peer and will destroy the QuicSession - // handler immediately. + state.idleTimeout = + Boolean(handle.state[IDX_QUIC_SESSION_STATE_IDLE_TIMEOUT]); + + // Destroy the underlying QuicSession handle handle.destroy(state.closeCode, state.closeFamily); } else { process.nextTick(emit.bind(this, 'close')); @@ -2108,7 +2058,8 @@ class QuicSession extends EventEmitter { const state = this[kInternalState]; return { code: state.closeCode, - family: state.closeFamily + family: state.closeFamily, + silent: state.silentClose, }; } diff --git a/src/quic/node_quic_session-inl.h b/src/quic/node_quic_session-inl.h index 339df8210f4a09..73db45ca56deb1 100644 --- a/src/quic/node_quic_session-inl.h +++ b/src/quic/node_quic_session-inl.h @@ -348,7 +348,7 @@ void QuicSession::OnIdleTimeout() { if (!is_destroyed()) { state_[IDX_QUIC_SESSION_STATE_IDLE_TIMEOUT] = 1; Debug(this, "Idle timeout"); - SilentClose(); + Close(QuicSessionListener::SESSION_CLOSE_FLAG_SILENT); } } diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index 5f102eb2f9362f..8263abc3cc1cab 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -1258,7 +1258,7 @@ bool QuicApplication::SendPendingData() { // to be silent because we can't even send a // CONNECTION_CLOSE since even those require a // packet number. - session()->SilentClose(); + session()->Close(QuicSessionListener::SESSION_CLOSE_FLAG_SILENT); return false; case NGTCP2_ERR_STREAM_DATA_BLOCKED: session()->StreamDataBlocked(stream_data.id); @@ -1629,59 +1629,6 @@ void QuicSession::AddStream(BaseObjectPtr stream) { } } -// Like the silent close, the immediate close must start with -// the JavaScript side, first shutting down any existing -// streams before entering the closing period. Unlike silent -// close, however, all streams are closed using proper -// STOP_SENDING and RESET_STREAM frames and a CONNECTION_CLOSE -// frame is ultimately sent to the peer. This makes the -// naming a bit of a misnomer in that the connection is -// not immediately torn down, but is allowed to drain -// properly per the QUIC spec description of "immediate close". -void QuicSession::ImmediateClose() { - // If ImmediateClose or SilentClose has already been called, - // do not re-enter. - if (is_closing()) - return; - set_closing(); - - QuicError err = last_error(); - Debug(this, "Immediate close with code %" PRIu64 " (%s)", - err.code, - err.family_name()); - - listener()->OnSessionClose(err); -} - -// Silent Close must start with the JavaScript side, which must -// clean up state, abort any still existing QuicSessions, then -// destroy the handle when done. The most important characteristic -// of the SilentClose is that no frames are sent to the peer. -// -// When a valid stateless reset is received, the connection is -// immediately and unrecoverably closed at the ngtcp2 level. -// Specifically, it will be put into the draining_period so -// absolutely no frames can be sent. What we need to do is -// notify the JavaScript side and destroy the connection with -// a flag set that indicates stateless reset. -void QuicSession::SilentClose() { - CHECK(!is_silent_closing()); - set_silent_closing(); - set_closing(); - - QuicError err = last_error(); - Debug(this, - "Silent close with %s code %" PRIu64 " (stateless reset? %s)", - err.family_name(), - err.code, - is_stateless_reset() ? "yes" : "no"); - - int flags = QuicSessionListener::SESSION_CLOSE_FLAG_SILENT; - if (is_stateless_reset()) - flags |= QuicSessionListener::SESSION_CLOSE_FLAG_STATELESS_RESET; - listener()->OnSessionClose(err, flags); -} - // Creates a new stream object and passes it off to the javascript side. // This has to be called from within a handlescope/contextscope. BaseObjectPtr QuicSession::CreateStream(int64_t stream_id) { @@ -1695,32 +1642,80 @@ BaseObjectPtr QuicSession::CreateStream(int64_t stream_id) { return stream; } -// Mark the QuicSession instance destroyed. After this is called, -// the QuicSession instance will be generally unusable but most -// likely will not be immediately freed. -void QuicSession::Destroy() { - if (is_destroyed()) +// Initiate a shutdown of the QuicSession. +void QuicSession::Close(int close_flags) { + CHECK(!is_destroyed()); + bool silent = close_flags & QuicSessionListener::SESSION_CLOSE_FLAG_SILENT; + bool stateless_reset = is_stateless_reset(); + + // If we're not running within a ngtcp2 callback scope, schedule + // a CONNECTION_CLOSE to be sent when Close exits. If we are + // within a ngtcp2 callback scope, sending the CONNECTION_CLOSE + // will be deferred. + ConnectionCloseScope close_scope(this, silent); + + // Once Close has been called, we cannot re-enter + if (UNLIKELY(is_closing())) return; - // If we're not in the closing or draining periods, - // then we should at least attempt to send a connection - // close to the peer. - if (!Ngtcp2CallbackScope::InNgtcp2CallbackScope(this) && - !is_in_closing_period() && - !is_in_draining_period()) { - Debug(this, "Making attempt to send a connection close"); - set_last_error(); - SendConnectionClose(); - } + set_closing(); + set_silent_closing(silent); - // Streams should have already been destroyed by this point. - CHECK(streams_.empty()); + if (stateless_reset && silent) + close_flags |= QuicSessionListener::SESSION_CLOSE_FLAG_STATELESS_RESET; + + QuicError error = last_error(); + Debug(this, "Closing with code %" PRIu64 + " (family: %s, silent: %s, stateless reset: %s)", + error.code, + error.family_name(), + silent ? "Y" : "N", + stateless_reset ? "Y" : "N"); + + // Ensure that the QuicSession is not freed at least until after we + // exit this scope. + BaseObjectPtr ptr(this); + + // If the QuicSession has been wrapped by a JS object, we have to + // notify the JavaScript side that the session is being closed. + // If it hasn't yet been wrapped, we can skip the call and and + // go straight to destroy. + if (is_wrapped()) + listener()->OnSessionClose(error, close_flags); + else + Destroy(); + + // At this point, the QuicSession should have been destroyed, indicating + // that all cleanup on the JavaScript side has completed and the + // QuicSession::Destroy() method has been called. + CHECK(is_destroyed()); +} + +// Mark the QuicSession instance destroyed. This will either be invoked +// synchronously within the callstack of the QuicSession::Close() method +// or not. If it is invoked within QuicSession::Close(), the +// QuicSession::Close() will handle sending the CONNECTION_CLOSE +// frame. +void QuicSession::Destroy() { + if (is_destroyed()) + return; // Mark the session destroyed. set_destroyed(); set_closing(false); set_graceful_closing(false); + // TODO(@jasnell): Allow overriding the close code + + // If we're not already in a ConnectionCloseScope, schedule + // sending a CONNECTION_CLOSE when destroy exits. If we are + // running within an ngtcp2 callback scope, sending the + // CONNECTION_CLOSE will be deferred. + ConnectionCloseScope close_scope(this, is_silent_closing()); + + // All existing streams should have already been destroyed + CHECK(streams_.empty()); + // Stop and free the idle and retransmission timers if they are active. StopIdleTimer(); StopRetransmitTimer(); @@ -1731,7 +1726,8 @@ void QuicSession::Destroy() { // the QuicSession from the QuicSocket will free // that pointer, allowing the QuicSession to be // deconstructed once the stack unwinds and any - // remaining shared_ptr instances fall out of scope. + // remaining BaseObjectPtr instances + // fall out of scope. RemoveFromSocket(); } @@ -1758,7 +1754,7 @@ void QuicSession::HandleError() { if (!SendConnectionClose()) { set_last_error(QUIC_ERROR_SESSION, NGTCP2_ERR_INTERNAL); - ImmediateClose(); + Close(); } } @@ -1933,7 +1929,6 @@ bool QuicSession::Receive( // in the closing period, a CONNECTION_CLOSE has not yet // been sent to the peer. Let's attempt to send one. if (!is_in_closing_period() && !is_in_draining_period()) { - Debug(this, "Attempting to send connection close"); set_last_error(); SendConnectionClose(); } @@ -1949,7 +1944,7 @@ bool QuicSession::Receive( // absolutely nothing left for us to do except silently close // and destroy this QuicSession. GetConnectionCloseInfo(); - SilentClose(); + Close(QuicSessionListener::SESSION_CLOSE_FLAG_SILENT); return true; } Debug(this, "Sending pending data after processing packet"); @@ -1991,7 +1986,7 @@ bool QuicSession::ReceivePacket( // then immediately close the connection. if (err == NGTCP2_ERR_RETRY && is_server()) { socket()->SendRetry(scid_, dcid_, local_address_, remote_address_); - SilentClose(); + Close(QuicSessionListener::SESSION_CLOSE_FLAG_SILENT); break; } set_last_error(QUIC_ERROR_SESSION, err); @@ -2154,35 +2149,17 @@ bool QuicSession::SendConnectionClose() { return true; } + UpdateIdleTimer(); switch (crypto_context_->side()) { case NGTCP2_CRYPTO_SIDE_SERVER: { - // If we're not already in the closing period, - // first attempt to write any pending packets, then - // start the closing period. If closing period has - // already started, skip this. - if (!is_in_closing_period() && - (!WritePackets("server connection close - write packets") || - !StartClosingPeriod())) { - return false; - } - - UpdateIdleTimer(); + if (!is_in_closing_period() && !StartClosingPeriod()) + return false; CHECK_GT(conn_closebuf_->length(), 0); - return SendPacket(QuicPacket::Copy(conn_closebuf_)); } case NGTCP2_CRYPTO_SIDE_CLIENT: { - UpdateIdleTimer(); auto packet = QuicPacket::Create("client connection close"); - // If we're not already in the closing period, - // first attempt to write any pending packets, then - // start the closing period. Note that the behavior - // here is different than the server - if (!is_in_closing_period() && - !WritePackets("client connection close - write packets")) { - return false; - } ssize_t nwrite = SelectCloseFn(error.family)( connection(), @@ -2191,7 +2168,7 @@ bool QuicSession::SendConnectionClose() { max_pktlen_, error.code, uv_hrtime()); - if (nwrite < 0) { + if (UNLIKELY(nwrite < 0)) { Debug(this, "Error writing connection close: %d", nwrite); set_last_error(QUIC_ERROR_SESSION, static_cast(nwrite)); return false; @@ -2233,7 +2210,6 @@ void QuicSession::UsePreferredAddressStrategy( // Passes a serialized packet to the associated QuicSocket. bool QuicSession::SendPacket(std::unique_ptr packet) { - CHECK(!is_destroyed()); CHECK(!is_in_draining_period()); // There's nothing to send. @@ -2391,7 +2367,7 @@ bool QuicSession::StartClosingPeriod() { if (nwrite < 0) { if (nwrite == NGTCP2_ERR_PKT_NUM_EXHAUSTED) { set_last_error(QUIC_ERROR_SESSION, NGTCP2_ERR_PKT_NUM_EXHAUSTED); - SilentClose(); + Close(QuicSessionListener::SESSION_CLOSE_FLAG_SILENT); } else { set_last_error(QUIC_ERROR_SESSION, static_cast(nwrite)); } @@ -2516,16 +2492,11 @@ void QuicSession::UpdateIdleTimer() { bool QuicSession::WritePackets(const char* diagnostic_label) { CHECK(!Ngtcp2CallbackScope::InNgtcp2CallbackScope(this)); - // During the draining period, we must not send any frames at all. - if (is_in_draining_period()) + // During either the draining or closing period, + // we are not permitted to send any additional packets. + if (is_in_draining_period() || is_in_closing_period()) return true; - // During the closing period, we are only permitted to send - // CONNECTION_CLOSE frames. - if (is_in_closing_period()) { - return SendConnectionClose(); - } - // Otherwise, serialize and send pending frames QuicPathStorage path; for (;;) { @@ -2541,9 +2512,11 @@ bool QuicSession::WritePackets(const char* diagnostic_label) { packet->data(), max_pktlen_, uv_hrtime()); + if (nwrite <= 0) { switch (nwrite) { case 0: + // There was nothing to write. return true; case NGTCP2_ERR_PKT_NUM_EXHAUSTED: // There is a finite number of packets that can be sent @@ -2553,7 +2526,7 @@ bool QuicSession::WritePackets(const char* diagnostic_label) { // to be silent because we can't even send a // CONNECTION_CLOSE since even those require a // packet number. - SilentClose(); + Close(QuicSessionListener::SESSION_CLOSE_FLAG_SILENT); return false; default: set_last_error(QUIC_ERROR_SESSION, static_cast(nwrite)); @@ -3359,23 +3332,6 @@ void QuicSessionSetSocket(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(session->set_socket(socket)); } -// Perform an immediate close on the QuicSession, causing a -// CONNECTION_CLOSE frame to be scheduled and sent and starting -// the closing period for this session. The name "ImmediateClose" -// is a bit of an unfortunate misnomer as the session will not -// be immediately shutdown. The naming is pulled from the QUIC -// spec to indicate a state where the session immediately enters -// the closing period, but the session will not be destroyed -// until either the idle timeout fires or destroy is explicitly -// called. -void QuicSessionClose(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - QuicSession* session; - ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - session->set_last_error(QuicError(env, args[0], args[1])); - session->SendConnectionClose(); -} - // GracefulClose flips a flag that prevents new local streams // from being opened and new remote streams from being received. It is // important to note that this does *NOT* send a CONNECTION_CLOSE packet @@ -3445,7 +3401,7 @@ void QuicSessionSilentClose(const FunctionCallbackInfo& args) { ProcessEmitWarning( session->env(), "Forcing silent close of QuicSession for testing purposes only"); - session->SilentClose(); + session->Close(QuicSessionListener::SESSION_CLOSE_FLAG_SILENT); } // TODO(addaleax): This is a temporary solution for testing and should be @@ -3592,7 +3548,6 @@ void NewQuicClientSession(const FunctionCallbackInfo& args) { // Add methods that are shared by both client and server QuicSessions void AddMethods(Environment* env, Local session) { - env->SetProtoMethod(session, "close", QuicSessionClose); env->SetProtoMethod(session, "destroy", QuicSessionDestroy); env->SetProtoMethod(session, "getRemoteAddress", QuicSessionGetRemoteAddress); env->SetProtoMethod(session, "getCertificate", QuicSessionGetCertificate); diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index 3087ecf12c947b..c68291aed67858 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -691,11 +691,13 @@ class QuicApplication : public MemoryRetainer, // QUICSESSION_FLAGS are converted into is_{name}() and set_{name}(bool on) // accessors on the QuicSession class. #define QUICSESSION_FLAGS(V) \ + V(WRAPPED, wrapped) \ V(CLOSING, closing) \ V(GRACEFUL_CLOSING, graceful_closing) \ V(DESTROYED, destroyed) \ V(TRANSPORT_PARAMS_SET, transport_params_set) \ V(NGTCP2_CALLBACK, in_ngtcp2_callback) \ + V(CONNECTION_CLOSE_SCOPE, in_connection_close_scope) \ V(SILENT_CLOSE, silent_closing) \ V(STATELESS_RESET, stateless_reset) @@ -847,7 +849,7 @@ class QuicSession : public AsyncWrap, #undef V // Returns true if the QuicSession has entered the - // closing period following a call to ImmediateClose. + // closing period after sending a CONNECTION_CLOSE. // While true, the QuicSession is only permitted to // transmit CONNECTION_CLOSE frames until either the // idle timeout period elapses or until the QuicSession @@ -871,7 +873,7 @@ class QuicSession : public AsyncWrap, // be immediately closed once there are no remaining streams. Note // that no notification is given to the connecting peer that we're // in a graceful closing state. A CONNECTION_CLOSE will be sent only - // once ImmediateClose() is called. + // once Close() is called. inline void StartGracefulClose(); QuicError last_error() const { return last_error_; } @@ -1040,17 +1042,16 @@ class QuicSession : public AsyncWrap, inline void DecreaseAllocatedSize(size_t size); - // Triggers an "immediate close" on the QuicSession. - // This will round trip through JavaScript, causing - // all currently open streams to be closed and ultimately - // send a CONNECTION_CLOSE to the connected peer before - // terminating the connection. - void ImmediateClose(); - - // Silently and immediately close the QuicSession. This is - // typically only done during an idle timeout or when sending - // a retry packet. - void SilentClose(); + // Initiate closing of the QuicSession. This will round trip + // through JavaScript, causing all currently opened streams + // to be closed. If the SESSION_CLOSE_FLAG_SILENT flag is + // set, the connected peer will not be notified, otherwise + // an attempt will be made to send a CONNECTION_CLOSE frame + // to the peer. If Close is called while within the ngtcp2 + // callback scope, sending the CONNECTION_CLOSE will be + // deferred until the ngtcp2 callback scope exits. + inline void Close( + int close_flags = QuicSessionListener::SESSION_CLOSE_FLAG_NONE); void PushListener(QuicSessionListener* listener); @@ -1087,12 +1088,48 @@ class QuicSession : public AsyncWrap, } ~SendSessionScope() { - if (!Ngtcp2CallbackScope::InNgtcp2CallbackScope(session_.get())) - session_->SendPendingData(); + if (Ngtcp2CallbackScope::InNgtcp2CallbackScope(session_.get()) || + session_->is_in_closing_period() || + session_->is_in_draining_period()) { + return; + } + session_->SendPendingData(); + } + + private: + BaseObjectPtr session_; + }; + + // ConnectionCloseScope triggers sending a CONNECTION_CLOSE + // when not executing within the context of an ngtcp2 callback + // and the session is in the correct state. + class ConnectionCloseScope { + public: + ConnectionCloseScope(QuicSession* session, bool silent = false) + : session_(session), + silent_(silent) { + CHECK(session_); + // If we are already in a ConnectionCloseScope, ignore. + if (session_->is_in_connection_close_scope()) + silent_ = true; + else + session_->set_in_connection_close_scope(); + } + + ~ConnectionCloseScope() { + if (silent_ || + Ngtcp2CallbackScope::InNgtcp2CallbackScope(session_.get()) || + session_->is_in_closing_period() || + session_->is_in_draining_period()) { + return; + } + session_->set_in_connection_close_scope(false); + bool ret = session_->SendConnectionClose(); } private: BaseObjectPtr session_; + bool silent_ = false; }; // Tracks whether or not we are currently within an ngtcp2 callback diff --git a/src/quic/node_quic_socket.cc b/src/quic/node_quic_socket.cc index 6ebdb30ef8a250..b51edee6c9be9a 100644 --- a/src/quic/node_quic_socket.cc +++ b/src/quic/node_quic_socket.cc @@ -843,6 +843,8 @@ BaseObjectPtr QuicSocket::AcceptInitialPacket( local_addr, remote_addr, NGTCP2_CONNECTION_REFUSED); + } else { + session->set_wrapped(); } return session; From 3acdd6aac74b7004a5be93d0baf7da789300a51d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 1 Jul 2020 16:40:16 -0700 Subject: [PATCH 05/25] quic: refactor QuicSession shared state to use AliasedStruct PR-URL: https://github.com/nodejs/node/pull/34160 Reviewed-By: Anna Henningsen --- lib/internal/quic/core.js | 49 ++++++------ lib/internal/quic/util.js | 133 ++++++++++++++++++++++++++++--- src/quic/node_quic.cc | 16 ++-- src/quic/node_quic_session-inl.h | 14 ++-- src/quic/node_quic_session.cc | 29 +++---- src/quic/node_quic_session.h | 96 +++++++--------------- 6 files changed, 196 insertions(+), 141 deletions(-) diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index 75f07c8d7e5871..00907398b09b8d 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -39,6 +39,7 @@ const { validateQuicEndpointOptions, validateCreateSecureContextOptions, validateQuicSocketConnectOptions, + QuicSessionSharedState, } = require('internal/quic/util'); const util = require('util'); const assert = require('internal/assert'); @@ -131,12 +132,6 @@ const { AF_INET, AF_INET6, NGTCP2_DEFAULT_MAX_PKTLEN, - IDX_QUIC_SESSION_STATE_MAX_STREAMS_BIDI, - IDX_QUIC_SESSION_STATE_MAX_STREAMS_UNI, - IDX_QUIC_SESSION_STATE_MAX_DATA_LEFT, - IDX_QUIC_SESSION_STATE_HANDSHAKE_CONFIRMED, - IDX_QUIC_SESSION_STATE_IDLE_TIMEOUT, - IDX_QUIC_SESSION_STATE_BYTES_IN_FLIGHT, IDX_QUIC_SESSION_STATS_CREATED_AT, IDX_QUIC_SESSION_STATS_HANDSHAKE_START_AT, IDX_QUIC_SESSION_STATS_BYTES_RECEIVED, @@ -605,11 +600,11 @@ function createSecureContext(options, init_cb) { } function onNewListener(event) { - toggleListeners(this[kHandle], event, true); + toggleListeners(this[kInternalState].state, event, true); } function onRemoveListener(event) { - toggleListeners(this[kHandle], event, false); + toggleListeners(this[kInternalState].state, event, false); } function getStats(obj, idx) { @@ -1651,6 +1646,7 @@ class QuicSession extends EventEmitter { handshakeContinuationHistogram: undefined, highWaterMark: undefined, defaultEncoding: undefined, + state: undefined, }; constructor(socket, options) { @@ -1693,6 +1689,7 @@ class QuicSession extends EventEmitter { this[kHandle] = handle; if (handle !== undefined) { handle[owner_symbol] = this; + state.state = new QuicSessionSharedState(handle.state); state.handshakeAckHistogram = new Histogram(handle.ack); state.handshakeContinuationHistogram = new Histogram(handle.rate); } else { @@ -1849,10 +1846,10 @@ class QuicSession extends EventEmitter { return false; } - // Closing allows any existing QuicStream's to complete - // normally but disallows any new QuicStreams from being - // opened. Calls to openStream() will fail, and new streams - // from the peer will be rejected/ignored. + // Closing allows any existing QuicStream's to gracefully + // complete while disallowing any new QuicStreams from being + // opened (in either direction). Calls to openStream() will + // fail, and new streams from the peer will be rejected/ignored. close(callback) { const state = this[kInternalState]; if (state.destroyed) @@ -1921,8 +1918,7 @@ class QuicSession extends EventEmitter { if (handle !== undefined) { // Copy the stats for use after destruction state.stats = new BigInt64Array(handle.stats); - state.idleTimeout = - Boolean(handle.state[IDX_QUIC_SESSION_STATE_IDLE_TIMEOUT]); + state.idleTimeout = this[kInternalState].state.idleTimeout; // Destroy the underlying QuicSession handle handle.destroy(state.closeCode, state.closeFamily); @@ -1950,8 +1946,8 @@ class QuicSession extends EventEmitter { let bidi = 0; let uni = 0; if (this[kHandle]) { - bidi = this[kHandle].state[IDX_QUIC_SESSION_STATE_MAX_STREAMS_BIDI]; - uni = this[kHandle].state[IDX_QUIC_SESSION_STATE_MAX_STREAMS_UNI]; + bidi = this[kInternalState].state.maxStreamsBidi; + uni = this[kInternalState].state.maxStreamsUni; } return { bidi, uni }; } @@ -1961,15 +1957,15 @@ class QuicSession extends EventEmitter { } get maxDataLeft() { - return this[kHandle]?.state[IDX_QUIC_SESSION_STATE_MAX_DATA_LEFT] || 0; + return this[kHandle] ? this[kInternalState].state.maxDataLeft : 0; } get bytesInFlight() { - return this[kHandle]?.state[IDX_QUIC_SESSION_STATE_BYTES_IN_FLIGHT] || 0; + return this[kHandle] ? this[kInternalState].state.bytesInFlight : 0; } get blockCount() { - return this[kHandle]?.state[IDX_QUIC_SESSION_STATS_BLOCK_COUNT] || 0; + return this[kHandle]?.stats[IDX_QUIC_SESSION_STATS_BLOCK_COUNT] || 0; } get authenticated() { @@ -2003,8 +1999,9 @@ class QuicSession extends EventEmitter { } get handshakeConfirmed() { - return Boolean( - this[kHandle]?.state[IDX_QUIC_SESSION_STATE_HANDSHAKE_CONFIRMED]); + return this[kHandle] ? + this[kInternalState].state.handshakeConfirmed : + false; } get idleTimeout() { @@ -2449,14 +2446,16 @@ class QuicClientSession extends QuicSession { // Listeners may have been added before the handle was created. // Ensure that we toggle those listeners in the handle state. - if (this.listenerCount('keylog') > 0) - toggleListeners(handle, 'keylog', true); + const internalState = this[kInternalState]; + if (this.listenerCount('keylog') > 0) { + toggleListeners(internalState.state, 'keylog', true); + } if (this.listenerCount('pathValidation') > 0) - toggleListeners(handle, 'pathValidation', true); + toggleListeners(internalState.state, 'pathValidation', true); if (this.listenerCount('usePreferredAddress') > 0) - toggleListeners(handle, 'usePreferredAddress', true); + toggleListeners(internalState.state, 'usePreferredAddress', true); this[kMaybeReady](0x2); } diff --git a/lib/internal/quic/util.js b/lib/internal/quic/util.js index efeddee94b6e64..6bbe10959723a9 100644 --- a/lib/internal/quic/util.js +++ b/lib/internal/quic/util.js @@ -15,6 +15,12 @@ const { }, } = require('internal/errors'); +const { + kHandle, +} = require('internal/stream_base_commons'); + +const endianness = require('os').endianness(); + const assert = require('internal/assert'); assert(process.versions.ngtcp2 !== undefined); @@ -52,11 +58,19 @@ const { IDX_QUIC_SESSION_MAX_UDP_PAYLOAD_SIZE, IDX_QUIC_SESSION_CC_ALGO, IDX_QUIC_SESSION_CONFIG_COUNT, - IDX_QUIC_SESSION_STATE_CERT_ENABLED, - IDX_QUIC_SESSION_STATE_CLIENT_HELLO_ENABLED, - IDX_QUIC_SESSION_STATE_KEYLOG_ENABLED, - IDX_QUIC_SESSION_STATE_PATH_VALIDATED_ENABLED, - IDX_QUIC_SESSION_STATE_USE_PREFERRED_ADDRESS_ENABLED, + + IDX_QUICSESSION_STATE_KEYLOG_ENABLED, + IDX_QUICSESSION_STATE_CLIENT_HELLO_ENABLED, + IDX_QUICSESSION_STATE_CERT_ENABLED, + IDX_QUICSESSION_STATE_PATH_VALIDATED_ENABLED, + IDX_QUICSESSION_STATE_USE_PREFERRED_ADDRESS_ENABLED, + IDX_QUICSESSION_STATE_HANDSHAKE_CONFIRMED, + IDX_QUICSESSION_STATE_IDLE_TIMEOUT, + IDX_QUICSESSION_STATE_MAX_STREAMS_BIDI, + IDX_QUICSESSION_STATE_MAX_STREAMS_UNI, + IDX_QUICSESSION_STATE_MAX_DATA_LEFT, + IDX_QUICSESSION_STATE_BYTES_IN_FLIGHT, + IDX_HTTP3_QPACK_MAX_TABLE_CAPACITY, IDX_HTTP3_QPACK_BLOCKED_STREAMS, IDX_HTTP3_MAX_HEADER_LIST_SIZE, @@ -756,29 +770,121 @@ function setTransportParams(config) { // communicate that a handler has been added for the optional events // so that the C++ internals know there is an actual listener. The event // will not be emitted if there is no handler. -function toggleListeners(handle, event, on) { - if (handle === undefined) +function toggleListeners(state, event, on) { + if (state === undefined) return; - const val = on ? 1 : 0; switch (event) { case 'keylog': - handle.state[IDX_QUIC_SESSION_STATE_KEYLOG_ENABLED] = val; + state.keylogEnabled = on; break; case 'clientHello': - handle.state[IDX_QUIC_SESSION_STATE_CLIENT_HELLO_ENABLED] = val; + state.clientHelloEnabled = on; break; case 'pathValidation': - handle.state[IDX_QUIC_SESSION_STATE_PATH_VALIDATED_ENABLED] = val; + state.pathValidatedEnabled = on; break; case 'OCSPRequest': - handle.state[IDX_QUIC_SESSION_STATE_CERT_ENABLED] = val; + state.certEnabled = on; break; case 'usePreferredAddress': - handle.state[IDX_QUIC_SESSION_STATE_USE_PREFERRED_ADDRESS_ENABLED] = on; + state.usePreferredAddressEnabled = on; break; } } +// A utility class used to handle reading / modifying shared JS/C++ +// state associated with a QuicSession +class QuicSessionSharedState { + constructor(state) { + this[kHandle] = Buffer.from(state); + } + + get keylogEnabled() { + return Boolean(this[kHandle] + .readUInt8(IDX_QUICSESSION_STATE_KEYLOG_ENABLED)); + } + + set keylogEnabled(on) { + this[kHandle] + .writeUInt8(on ? 1 : 0, IDX_QUICSESSION_STATE_KEYLOG_ENABLED); + } + + get clientHelloEnabled() { + return Boolean(this[kHandle] + .readUInt8(IDX_QUICSESSION_STATE_CLIENT_HELLO_ENABLED)); + } + + set clientHelloEnabled(on) { + this[kHandle] + .writeUInt8(on ? 1 : 0, IDX_QUICSESSION_STATE_CLIENT_HELLO_ENABLED); + } + + get certEnabled() { + return Boolean(this[kHandle] + .readUInt8(IDX_QUICSESSION_STATE_CERT_ENABLED)); + } + + set certEnabled(on) { + this[kHandle] + .writeUInt8(on ? 1 : 0, IDX_QUICSESSION_STATE_CERT_ENABLED); + } + + get pathValidatedEnabled() { + return Boolean(this[kHandle] + .readUInt8(IDX_QUICSESSION_STATE_PATH_VALIDATED_ENABLED)); + } + + set pathValidatedEnabled(on) { + this[kHandle] + .writeUInt8(on ? 1 : 0, IDX_QUICSESSION_STATE_PATH_VALIDATED_ENABLED); + } + + get usePreferredAddressEnabled() { + return Boolean(this[kHandle] + .readUInt8(IDX_QUICSESSION_STATE_USE_PREFERRED_ADDRESS_ENABLED)); + } + + set usePreferredAddressEnabled(on) { + this[kHandle] + .writeUInt8(on ? 1 : 0, + IDX_QUICSESSION_STATE_USE_PREFERRED_ADDRESS_ENABLED); + } + + get handshakeConfirmed() { + return Boolean(this[kHandle] + .readUInt8(IDX_QUICSESSION_STATE_HANDSHAKE_CONFIRMED)); + } + + get idleTimeout() { + return Boolean(this[kHandle] + .readUInt8(IDX_QUICSESSION_STATE_IDLE_TIMEOUT)); + } + + get maxStreamsBidi() { + return Number(endianness === 'BE' ? + this[kHandle].readBigInt64BE(IDX_QUICSESSION_STATE_MAX_STREAMS_BIDI) : + this[kHandle].readBigInt64LE(IDX_QUICSESSION_STATE_MAX_STREAMS_BIDI)); + } + + get maxStreamsUni() { + return Number(endianness === 'BE' ? + this[kHandle].readBigInt64BE(IDX_QUICSESSION_STATE_MAX_STREAMS_UNI) : + this[kHandle].readBigInt64LE(IDX_QUICSESSION_STATE_MAX_STREAMS_UNI)); + } + + get maxDataLeft() { + return Number(endianness === 'BE' ? + this[kHandle].readBigInt64BE(IDX_QUICSESSION_STATE_MAX_DATA_LEFT) : + this[kHandle].readBigInt64LE(IDX_QUICSESSION_STATE_MAX_DATA_LEFT)); + } + + get bytesInFlight() { + return Number(endianness === 'BE' ? + this[kHandle].readBigInt64BE(IDX_QUICSESSION_STATE_BYTES_IN_FLIGHT) : + this[kHandle].readBigInt64LE(IDX_QUICSESSION_STATE_BYTES_IN_FLIGHT)); + } +} + module.exports = { getAllowUnauthorized, getSocketType, @@ -796,4 +902,5 @@ module.exports = { validateQuicEndpointOptions, validateCreateSecureContextOptions, validateQuicSocketConnectOptions, + QuicSessionSharedState, }; diff --git a/src/quic/node_quic.cc b/src/quic/node_quic.cc index ed147eb92b649f..49aba2e5db2870 100644 --- a/src/quic/node_quic.cc +++ b/src/quic/node_quic.cc @@ -173,17 +173,6 @@ void Initialize(Local target, V(IDX_QUIC_SESSION_MAX_ACK_DELAY) \ V(IDX_QUIC_SESSION_CC_ALGO) \ V(IDX_QUIC_SESSION_CONFIG_COUNT) \ - V(IDX_QUIC_SESSION_STATE_CERT_ENABLED) \ - V(IDX_QUIC_SESSION_STATE_CLIENT_HELLO_ENABLED) \ - V(IDX_QUIC_SESSION_STATE_USE_PREFERRED_ADDRESS_ENABLED) \ - V(IDX_QUIC_SESSION_STATE_PATH_VALIDATED_ENABLED) \ - V(IDX_QUIC_SESSION_STATE_KEYLOG_ENABLED) \ - V(IDX_QUIC_SESSION_STATE_MAX_STREAMS_BIDI) \ - V(IDX_QUIC_SESSION_STATE_MAX_STREAMS_UNI) \ - V(IDX_QUIC_SESSION_STATE_MAX_DATA_LEFT) \ - V(IDX_QUIC_SESSION_STATE_BYTES_IN_FLIGHT) \ - V(IDX_QUIC_SESSION_STATE_HANDSHAKE_CONFIRMED) \ - V(IDX_QUIC_SESSION_STATE_IDLE_TIMEOUT) \ V(MAX_RETRYTOKEN_EXPIRATION) \ V(MIN_RETRYTOKEN_EXPIRATION) \ V(NGTCP2_APP_NOERROR) \ @@ -212,6 +201,11 @@ void Initialize(Local target, V(ERR_FAILED_TO_CREATE_SESSION) \ V(UV_EBADF) +#define V(id, _, __) \ + NODE_DEFINE_CONSTANT(constants, IDX_QUICSESSION_STATE_##id); + QUICSESSION_SHARED_STATE(V) +#undef V + #define V(name, _, __) \ NODE_DEFINE_CONSTANT(constants, IDX_QUIC_SESSION_STATS_##name); SESSION_STATS(V) diff --git a/src/quic/node_quic_session-inl.h b/src/quic/node_quic_session-inl.h index 73db45ca56deb1..935c8d024d55c9 100644 --- a/src/quic/node_quic_session-inl.h +++ b/src/quic/node_quic_session-inl.h @@ -105,7 +105,7 @@ ngtcp2_crypto_level QuicCryptoContext::write_crypto_level() const { // to a keylog file that can be consumed by tools like Wireshark to intercept // and decrypt QUIC network traffic. void QuicCryptoContext::Keylog(const char* line) { - if (UNLIKELY(session_->state_[IDX_QUIC_SESSION_STATE_KEYLOG_ENABLED] == 1)) + if (UNLIKELY(session_->state_->keylog_enabled)) session_->listener()->OnKeylog(line, strlen(line)); } @@ -117,7 +117,7 @@ void QuicCryptoContext::OnClientHelloDone() { [&]() { set_in_client_hello(false); }); // Disable the callback at this point so we don't loop continuously - session_->state_[IDX_QUIC_SESSION_STATE_CLIENT_HELLO_ENABLED] = 0; + session_->state_->client_hello_enabled = 0; } // Following a pause in the handshake for OCSP or client hello, we kickstart @@ -274,14 +274,12 @@ void QuicSession::ExtendMaxStreamsRemoteBidi(uint64_t max_streams) { void QuicSession::ExtendMaxStreamsUni(uint64_t max_streams) { Debug(this, "Setting max unidirectional streams to %" PRIu64, max_streams); - state_[IDX_QUIC_SESSION_STATE_MAX_STREAMS_UNI] = - static_cast(max_streams); + state_->max_streams_uni = max_streams; } void QuicSession::ExtendMaxStreamsBidi(uint64_t max_streams) { Debug(this, "Setting max bidirectional streams to %" PRIu64, max_streams); - state_[IDX_QUIC_SESSION_STATE_MAX_STREAMS_BIDI] = - static_cast(max_streams); + state_->max_streams_bidi = max_streams; } // Extends the stream-level flow control by the given number of bytes. @@ -327,7 +325,7 @@ void QuicSession::HandshakeCompleted() { void QuicSession::HandshakeConfirmed() { Debug(this, "Handshake is confirmed"); RecordTimestamp(&QuicSessionStats::handshake_confirmed_at); - state_[IDX_QUIC_SESSION_STATE_HANDSHAKE_CONFIRMED] = 1; + state_->handshake_confirmed = 1; } bool QuicSession::is_handshake_completed() const { @@ -346,7 +344,7 @@ void QuicSession::InitApplication() { // the peer. All existing streams are abandoned and closed. void QuicSession::OnIdleTimeout() { if (!is_destroyed()) { - state_[IDX_QUIC_SESSION_STATE_IDLE_TIMEOUT] = 1; + state_->idle_timeout = 1; Debug(this, "Idle timeout"); Close(QuicSessionListener::SESSION_CLOSE_FLAG_SILENT); } diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index 8263abc3cc1cab..ffc4f25bfcc9d8 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -1,5 +1,6 @@ #include "node_quic_session-inl.h" // NOLINT(build/include) #include "aliased_buffer.h" +#include "aliased_struct-inl.h" #include "allocated_buffer-inl.h" #include "debug_utils-inl.h" #include "env-inl.h" @@ -919,10 +920,8 @@ void QuicCryptoContext::EnableTrace() { // when the peer certificate is received, allowing additional tweaks // and verifications to be performed. int QuicCryptoContext::OnClientHello() { - if (LIKELY(session_->state_[ - IDX_QUIC_SESSION_STATE_CLIENT_HELLO_ENABLED] == 0)) { + if (LIKELY(session_->state_->client_hello_enabled == 0)) return 0; - } TLSCallbackScope callback_scope(this); @@ -952,7 +951,7 @@ int QuicCryptoContext::OnClientHello() { // function that must be called in order for the TLS handshake to // continue. int QuicCryptoContext::OnOCSP() { - if (LIKELY(session_->state_[IDX_QUIC_SESSION_STATE_CERT_ENABLED] == 0)) { + if (LIKELY(session_->state_->cert_enabled == 0)) { Debug(session(), "No OCSPRequest handler registered"); return 1; } @@ -991,7 +990,7 @@ void QuicCryptoContext::OnOCSPDone( [&]() { set_in_ocsp_request(false); }); // Disable the callback at this point so we don't loop continuously - session_->state_[IDX_QUIC_SESSION_STATE_CERT_ENABLED] = 0; + session_->state_->cert_enabled = 0; if (context) { int err = crypto::UseSNIContext(ssl_, context); @@ -1446,7 +1445,7 @@ QuicSession::QuicSession( idle_(new Timer(socket->env(), [this]() { OnIdleTimeout(); })), retransmit_(new Timer(socket->env(), [this]() { MaybeTimeout(); })), dcid_(dcid), - state_(env()->isolate(), IDX_QUIC_SESSION_STATE_COUNT), + state_(env()->isolate()), quic_state_(socket->quic_state()) { PushListener(&default_listener_); set_connection_id_strategy(RandomConnectionIDStrategy); @@ -1459,13 +1458,10 @@ QuicSession::QuicSession( options)); application_.reset(SelectApplication(this)); - // TODO(@jasnell): For now, the following is a check rather than properly - // handled. Before this code moves out of experimental, this should be - // properly handled. wrap->DefineOwnProperty( env()->context(), env()->state_string(), - state_.GetJSArray(), + state_.GetArrayBuffer(), PropertyAttribute::ReadOnly).Check(); // TODO(@jasnell): memory accounting @@ -1814,7 +1810,7 @@ void QuicSession::PathValidation( // Only emit the callback if there is a handler for the pathValidation // event on the JavaScript QuicSession object. - if (UNLIKELY(state_[IDX_QUIC_SESSION_STATE_PATH_VALIDATED_ENABLED] == 1)) { + if (UNLIKELY(state_->path_validated_enabled == 1)) { listener_->OnPathValidation( res, reinterpret_cast(path->local.addr), @@ -2190,14 +2186,12 @@ void QuicSession::IgnorePreferredAddressStrategy( void QuicSession::UsePreferredAddressStrategy( QuicSession* session, const PreferredAddress& preferred_address) { - static constexpr int idx = - IDX_QUIC_SESSION_STATE_USE_PREFERRED_ADDRESS_ENABLED; int family = session->socket()->local_address().family(); if (preferred_address.Use(family)) { Debug(session, "Using server preferred address"); // Emit only if the QuicSession has a usePreferredAddress handler // on the JavaScript side. - if (UNLIKELY(session->state_[idx] == 1)) { + if (UNLIKELY(session->state_->use_preferred_address_enabled == 1)) { session->listener()->OnUsePreferredAddress(family, preferred_address); } } else { @@ -2550,7 +2544,6 @@ void QuicSession::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("idle", idle_); tracker->TrackField("retransmit", retransmit_); tracker->TrackField("streams", streams_); - tracker->TrackField("state", state_); tracker->TrackFieldWithSize("current_ngtcp2_memory", current_ngtcp2_memory_); tracker->TrackField("conn_closebuf", conn_closebuf_); tracker->TrackField("application", application_); @@ -2697,14 +2690,12 @@ void QuicSession::UpdateRecoveryStats() { void QuicSession::UpdateDataStats() { if (is_destroyed()) return; - state_[IDX_QUIC_SESSION_STATE_MAX_DATA_LEFT] = - static_cast(ngtcp2_conn_get_max_data_left(connection())); + state_->max_data_left = ngtcp2_conn_get_max_data_left(connection()); ngtcp2_conn_stat stat; ngtcp2_conn_get_conn_stat(connection(), &stat); - state_[IDX_QUIC_SESSION_STATE_BYTES_IN_FLIGHT] = - static_cast(stat.bytes_in_flight); + state_->bytes_in_flight = stat.bytes_in_flight; // The max_bytes_in_flight is a highwater mark that can be used // in performance analysis operations. if (stat.bytes_in_flight > GetStat(&QuicSessionStats::max_bytes_in_flight)) diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index c68291aed67858..acf433622b38d3 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -4,6 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "aliased_buffer.h" +#include "aliased_struct.h" #include "async_wrap.h" #include "env.h" #include "handle_wrap.h" @@ -141,67 +142,32 @@ enum QuicClientSessionOptions : uint32_t { QUICCLIENTSESSION_OPTION_RESUME = 0x4 }; +#define QUICSESSION_SHARED_STATE(V) \ + V(KEYLOG_ENABLED, keylog_enabled, uint8_t) \ + V(CLIENT_HELLO_ENABLED, client_hello_enabled, uint8_t) \ + V(CERT_ENABLED, cert_enabled, uint8_t) \ + V(PATH_VALIDATED_ENABLED, path_validated_enabled, uint8_t) \ + V(USE_PREFERRED_ADDRESS_ENABLED, use_preferred_address_enabled, uint8_t) \ + V(HANDSHAKE_CONFIRMED, handshake_confirmed, uint8_t) \ + V(IDLE_TIMEOUT, idle_timeout, uint8_t) \ + V(MAX_STREAMS_BIDI, max_streams_bidi, uint64_t) \ + V(MAX_STREAMS_UNI, max_streams_uni, uint64_t) \ + V(MAX_DATA_LEFT, max_data_left, uint64_t) \ + V(BYTES_IN_FLIGHT, bytes_in_flight, uint64_t) + +#define V(_, name, type) type name; +struct QuicSessionState { + QUICSESSION_SHARED_STATE(V) +}; +#undef V -// The QuicSessionState enums are used with the QuicSession's -// private state_ array. This is exposed to JavaScript via an -// aliased buffer and is used to communicate various types of -// state efficiently across the native/JS boundary. -enum QuicSessionState : int { - // Communicates whether a 'keylog' event listener has been - // registered on the JavaScript QuicSession object. The - // value will be either 1 or 0. When set to 1, the native - // code will emit TLS keylog entries to the JavaScript - // side triggering the 'keylog' event once for each line. - IDX_QUIC_SESSION_STATE_KEYLOG_ENABLED, - - // Communicates whether a 'clientHello' event listener has - // been registered on the JavaScript QuicServerSession. - // The value will be either 1 or 0. When set to 1, the - // native code will callout to the JavaScript side causing - // the 'clientHello' event to be emitted. This is only - // used on server QuicSession instances. - IDX_QUIC_SESSION_STATE_CLIENT_HELLO_ENABLED, - - // Communicates whether a 'cert' event listener has been - // registered on the JavaScript QuicSession. The value will - // be either 1 or 0. When set to 1, the native code will - // callout to the JavaScript side causing the 'cert' event - // to be emitted. - IDX_QUIC_SESSION_STATE_CERT_ENABLED, - - // Communicates whether a 'pathValidation' event listener - // has been registered on the JavaScript QuicSession. The - // value will be either 1 or 0. When set to 1, the native - // code will callout to the JavaScript side causing the - // 'pathValidation' event to be emitted - IDX_QUIC_SESSION_STATE_PATH_VALIDATED_ENABLED, - - // Communicates the current max cumulative number of - // bidi and uni streams that may be opened on the session - IDX_QUIC_SESSION_STATE_MAX_STREAMS_BIDI, - IDX_QUIC_SESSION_STATE_MAX_STREAMS_UNI, - - // Communicates the current maxinum number of bytes that - // the local endpoint can send in this connection - // (updated immediately after processing sent/received packets) - IDX_QUIC_SESSION_STATE_MAX_DATA_LEFT, - - // Communicates the current total number of bytes in flight - IDX_QUIC_SESSION_STATE_BYTES_IN_FLIGHT, - - // Communicates whether a 'usePreferredAddress' event listener - // has been registered. - IDX_QUIC_SESSION_STATE_USE_PREFERRED_ADDRESS_ENABLED, - - IDX_QUIC_SESSION_STATE_HANDSHAKE_CONFIRMED, - - // Communicates whether a session was closed due to idle timeout - IDX_QUIC_SESSION_STATE_IDLE_TIMEOUT, - - // Just the number of session state enums for use when - // creating the AliasedBuffer. - IDX_QUIC_SESSION_STATE_COUNT +#define V(id, name, _) \ + IDX_QUICSESSION_STATE_##id = offsetof(QuicSessionState, name), +enum QuicSessionStateFields { + QUICSESSION_SHARED_STATE(V) + IDX_QUICSESSION_STATE_END }; +#undef V #define SESSION_STATS(V) \ V(CREATED_AT, created_at, "Created At") \ @@ -1163,9 +1129,9 @@ class QuicSession : public AsyncWrap, private: static void RandomConnectionIDStrategy( - QuicSession* session, - ngtcp2_cid* cid, - size_t cidlen); + QuicSession* session, + ngtcp2_cid* cid, + size_t cidlen); // Initialize the QuicSession as a server void InitServer( @@ -1219,8 +1185,8 @@ class QuicSession : public AsyncWrap, inline void HandshakeConfirmed(); void PathValidation( - const ngtcp2_path* path, - ngtcp2_path_validation_result res); + const ngtcp2_path* path, + ngtcp2_path_validation_result res); bool ReceivePacket(ngtcp2_path* path, const uint8_t* data, ssize_t nread); @@ -1484,7 +1450,7 @@ class QuicSession : public AsyncWrap, StreamsMap streams_; - AliasedFloat64Array state_; + AliasedStruct state_; struct RemoteTransportParamsDebug { QuicSession* session; From e4d369e96e0f75786c84dacb45f0a8790615b386 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 2 Jul 2020 07:07:14 -0700 Subject: [PATCH 06/25] quic: remove onSessionDestroy callback The QuicSession can be destroyed during garbage collection and the onSessionDestroy callback was happening in the destructor. PR-URL: https://github.com/nodejs/node/pull/34160 Reviewed-By: Anna Henningsen --- lib/internal/quic/core.js | 30 ++++--------------- src/env.h | 1 - src/quic/node_quic.cc | 1 - src/quic/node_quic_session.cc | 17 +---------- src/quic/node_quic_session.h | 2 -- .../test-quic-maxconnectionsperhost.js | 4 +-- 6 files changed, 9 insertions(+), 46 deletions(-) diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index 00907398b09b8d..babbdb39bcf6d9 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -288,21 +288,6 @@ function onSessionClose(code, family, silent, statelessReset) { this[owner_symbol][kDestroy](code, family, silent, statelessReset); } -// Called by the C++ internals when a QuicSession has been destroyed. -// When this is called, the QuicSession is no longer usable. Removing -// the handle and emitting close is the only action. -// TODO(@jasnell): In the future, this will need to act differently -// for QuicClientSessions when autoResume is enabled. -function onSessionDestroyed() { - const session = this[owner_symbol]; - this[owner_symbol] = undefined; - - if (session) { - session[kSetHandle](); - process.nextTick(emit.bind(session, 'close')); - } -} - // Used only within the onSessionClientHello function. Invoked // to complete the client hello process. function clientHelloCallback(err, ...args) { @@ -558,7 +543,6 @@ setCallbacks({ onSessionCert, onSessionClientHello, onSessionClose, - onSessionDestroyed, onSessionHandshake, onSessionKeylog, onSessionQlog, @@ -1908,13 +1892,8 @@ class QuicSession extends EventEmitter { this.removeListener('newListener', onNewListener); this.removeListener('removeListener', onRemoveListener); - // If we are destroying with an error, schedule the - // error to be emitted on process.nextTick. - if (error) process.nextTick(emit.bind(this, 'error', error)); - const handle = this[kHandle]; - this[kHandle] = undefined; - + this[kHandle] = undefined if (handle !== undefined) { // Copy the stats for use after destruction state.stats = new BigInt64Array(handle.stats); @@ -1922,14 +1901,17 @@ class QuicSession extends EventEmitter { // Destroy the underlying QuicSession handle handle.destroy(state.closeCode, state.closeFamily); - } else { - process.nextTick(emit.bind(this, 'close')); } // Remove the QuicSession JavaScript object from the // associated QuicSocket. state.socket[kRemoveSession](this); state.socket = undefined; + + // If we are destroying with an error, schedule the + // error to be emitted on process.nextTick. + if (error) process.nextTick(emit.bind(this, 'error', error)); + process.nextTick(emit.bind(this, 'close')); } // For server QuicSession instances, true if earlyData is diff --git a/src/env.h b/src/env.h index 823aab9f55a301..07e8307f056640 100644 --- a/src/env.h +++ b/src/env.h @@ -454,7 +454,6 @@ constexpr size_t kFsStatsBufferLength = V(quic_on_session_cert_function, v8::Function) \ V(quic_on_session_client_hello_function, v8::Function) \ V(quic_on_session_close_function, v8::Function) \ - V(quic_on_session_destroyed_function, v8::Function) \ V(quic_on_session_handshake_function, v8::Function) \ V(quic_on_session_keylog_function, v8::Function) \ V(quic_on_session_path_validation_function, v8::Function) \ diff --git a/src/quic/node_quic.cc b/src/quic/node_quic.cc index 49aba2e5db2870..a651c2f951ffdf 100644 --- a/src/quic/node_quic.cc +++ b/src/quic/node_quic.cc @@ -59,7 +59,6 @@ void QuicSetCallbacks(const FunctionCallbackInfo& args) { SETFUNCTION("onSessionCert", session_cert); SETFUNCTION("onSessionClientHello", session_client_hello); SETFUNCTION("onSessionClose", session_close); - SETFUNCTION("onSessionDestroyed", session_destroyed); SETFUNCTION("onSessionHandshake", session_handshake); SETFUNCTION("onSessionKeylog", session_keylog); SETFUNCTION("onSessionUsePreferredAddress", session_use_preferred_address); diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index ffc4f25bfcc9d8..0ac7d2ec6fa2a2 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -289,11 +289,6 @@ void QuicSessionListener::OnStreamReset( previous_listener_->OnStreamReset(stream_id, app_error_code); } -void QuicSessionListener::OnSessionDestroyed() { - if (previous_listener_ != nullptr) - previous_listener_->OnSessionDestroyed(); -} - void QuicSessionListener::OnSessionClose(QuicError error, int flags) { if (previous_listener_ != nullptr) previous_listener_->OnSessionClose(error, flags); @@ -509,16 +504,6 @@ void JSQuicSessionListener::OnStreamReset( argv); } -void JSQuicSessionListener::OnSessionDestroyed() { - Environment* env = session()->env(); - HandleScope scope(env->isolate()); - Context::Scope context_scope(env->context()); - // Emit the 'close' event in JS. This needs to happen after destroying the - // connection, because doing so also releases the last qlog data. - session()->MakeCallback( - env->quic_on_session_destroyed_function(), 0, nullptr); -} - void JSQuicSessionListener::OnSessionClose(QuicError error, int flags) { Environment* env = session()->env(); HandleScope scope(env->isolate()); @@ -1412,6 +1397,7 @@ QuicSession::QuicSession( QuicCID(), options, preferred_address_strategy) { + set_wrapped(); InitClient( local_addr, remote_addr, @@ -1472,7 +1458,6 @@ QuicSession::~QuicSession() { CHECK(!Ngtcp2CallbackScope::InNgtcp2CallbackScope(this)); QuicSessionListener* listener_ = listener(); - listener_->OnSessionDestroyed(); if (listener_ == listener()) RemoveListener(listener_); diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index acf433622b38d3..7556e2fc5878a3 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -251,7 +251,6 @@ class QuicSessionListener { virtual void OnStreamReset( int64_t stream_id, uint64_t app_error_code); - virtual void OnSessionDestroyed(); virtual void OnSessionClose( QuicError error, int flags = SESSION_CLOSE_FLAG_NONE); @@ -299,7 +298,6 @@ class JSQuicSessionListener : public QuicSessionListener { void OnStreamReset( int64_t stream_id, uint64_t app_error_code) override; - void OnSessionDestroyed() override; void OnSessionClose( QuicError error, int flags = SESSION_CLOSE_FLAG_NONE) override; diff --git a/test/parallel/test-quic-maxconnectionsperhost.js b/test/parallel/test-quic-maxconnectionsperhost.js index b94849b88272c3..1bfbb7b8e790ea 100644 --- a/test/parallel/test-quic-maxconnectionsperhost.js +++ b/test/parallel/test-quic-maxconnectionsperhost.js @@ -77,8 +77,8 @@ const kALPN = 'zzz'; countdown.dec(); // Shutdown the remaining open sessions. setImmediate(common.mustCall(() => { - for (const req of sessions) - req.close(); + for (const req of sessions) + req.close(); })); })); From 7b062ca015d5a2545c9d66992c183c325d938a23 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 2 Jul 2020 12:44:57 -0700 Subject: [PATCH 07/25] quic: refactor qlog handling Because of the timing of qlog events emitted by ngtcp2, it becomes difficult to handle those as events on the QuicSession object because the final qlog entry is not emitted until the ngtcp2_conn is freed, which can occur when the object is being garbage collected (meaning, we a: can't call out to javascript and b: don't have an object we can use to emit the event). This refactors it into a QLogStream object that allows the qlog data to be piped out using a separate Readable stream. PR-URL: https://github.com/nodejs/node/pull/34160 Reviewed-By: Anna Henningsen --- doc/api/quic.md | 45 ++++--- lib/internal/quic/core.js | 59 +++++---- lib/internal/quic/util.js | 38 ++++++ src/async_wrap.h | 1 + src/env.h | 1 + src/quic/node_quic_session.cc | 112 ++++++++++++++++-- src/quic/node_quic_session.h | 42 ++++++- test/parallel/test-quic-qlog.js | 22 +++- test/sequential/test-async-wrap-getasyncid.js | 1 + 9 files changed, 257 insertions(+), 64 deletions(-) diff --git a/doc/api/quic.md b/doc/api/quic.md index 42a8b756909d1e..3667a05b394956 100644 --- a/doc/api/quic.md +++ b/doc/api/quic.md @@ -274,7 +274,7 @@ added: REPLACEME * `maxStatelessResetsPerHost` {number} The maximum number of stateless resets that the `QuicSocket` is permitted to send per remote host. Default: `10`. - * `qlog` {boolean} Whether to emit ['qlog'][] events for incoming sessions. + * `qlog` {boolean} Whether to enable ['qlog'][] for incoming sessions. (For outgoing client sessions, set `client.qlog`.) Default: `false`. * `retryTokenTimeout` {number} The maximum number of *seconds* for retry token validation. Default: `10` seconds. @@ -633,20 +633,6 @@ The callback will be invoked with three arguments: The `'pathValidation'` event will be emitted multiple times. -#### Event: `'qlog'` - - -* `jsonChunk` {string} A JSON fragment. - -Emitted if the `qlog: true` option was passed to `quicsocket.connect()` or -`net.createQuicSocket()` functions. - -The argument is a JSON fragment according to the [qlog standard][]. - -The `'qlog'` event will be emitted multiple times. - #### Event: `'secure'` + +* Type: {stream.Readable} + +If `qlog` support is enabled for `QuicSession`, the `quicsession.qlog` property +provides a [`stream.Readable`][] that may be used to access the `qlog` event +data according to the [qlog standard][]. For client `QuicSessions`, the +`quicsession.qlog` property will be `undefined` untilt the `'qlog'` event +is emitted. + #### quicsession.remoteAddress + +The `'qlog'` event is emitted when the `QuicClientSession` is ready to begin +providing `qlog` event data. The callback is invoked with a single argument: + +* `qlog` {stream.Readable} A [`stream.Readable`][] that is also available using + the `quicsession.qlog` property. + #### Event: `'usePreferredAddress'` + +Used when a feature that is not available +to the current platform which is running Node.js is used. + ### `ERR_FS_FILE_TOO_LARGE` @@ -2670,12 +2679,6 @@ while trying to read and parse it. The `--entry-type=...` flag is not compatible with the Node.js REPL. - -#### `ERR_FEATURE_UNAVAILABLE_ON_PLATFORM` - -Used when a feature that is not available -to the current platform which is running Node.js is used. - #### `ERR_STREAM_HAS_STRINGDECODER` From 82c435d0a08423cec36c91df7335f417ffa41122 Mon Sep 17 00:00:00 2001 From: Nikola Glavina Date: Sun, 5 Jul 2020 23:37:01 +0200 Subject: [PATCH 18/25] src: fix unused namespace member C++ linter fails because of unused ArrayBuffer namespace member PR-URL: https://github.com/nodejs/node/pull/34212 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Jiawen Geng Reviewed-By: Richard Lau Reviewed-By: Gireesh Punathil --- src/env.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/env.cc b/src/env.cc index bfcee5a6a0f827..1ab9206ce48c8b 100644 --- a/src/env.cc +++ b/src/env.cc @@ -27,7 +27,6 @@ namespace node { using errors::TryCatchScope; -using v8::ArrayBuffer; using v8::Boolean; using v8::Context; using v8::EmbedderGraph; From 67ba825037b4082d5d16f922fb9ce54516b4a869 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Thu, 2 Jul 2020 12:51:00 +0200 Subject: [PATCH 19/25] src: fix minor comment typo in KeyObjectData MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/34167 Reviewed-By: Anna Henningsen Reviewed-By: Richard Lau Reviewed-By: Tobias Nießen Reviewed-By: David Carlier --- src/node_crypto.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_crypto.h b/src/node_crypto.h index a61089f0f131c0..63468e4f18dad1 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -420,7 +420,7 @@ class KeyObjectData { KeyType GetKeyType() const; // These functions allow unprotected access to the raw key material and should - // only be used to implement cryptograohic operations requiring the key. + // only be used to implement cryptographic operations requiring the key. ManagedEVPPKey GetAsymmetricKey() const; const char* GetSymmetricKey() const; size_t GetSymmetricKeySize() const; From ddfaafa9b0d94daf183390a08e8a4b11d05a65fd Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 4 Jul 2020 06:32:56 -0700 Subject: [PATCH 20/25] repl: fix verb conjugation in deprecation message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/34198 Reviewed-By: Michaël Zasso Reviewed-By: Ruben Bridgewater Reviewed-By: Zeyu Yang Reviewed-By: Trivikram Kamat Reviewed-By: James M Snell --- lib/repl.js | 8 ++++---- test/parallel/test-repl-options.js | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/repl.js b/lib/repl.js index a755dbf1bb107e..cc5bf069c31295 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -225,13 +225,13 @@ function REPLServer(prompt, ObjectDefineProperty(this, 'inputStream', { get: pendingDeprecation ? deprecate(() => this.input, - 'repl.inputStream and repl.outputStream is deprecated. ' + + 'repl.inputStream and repl.outputStream are deprecated. ' + 'Use repl.input and repl.output instead', 'DEP0141') : () => this.input, set: pendingDeprecation ? deprecate((val) => this.input = val, - 'repl.inputStream and repl.outputStream is deprecated. ' + + 'repl.inputStream and repl.outputStream are deprecated. ' + 'Use repl.input and repl.output instead', 'DEP0141') : (val) => this.input = val, @@ -241,13 +241,13 @@ function REPLServer(prompt, ObjectDefineProperty(this, 'outputStream', { get: pendingDeprecation ? deprecate(() => this.output, - 'repl.inputStream and repl.outputStream is deprecated. ' + + 'repl.inputStream and repl.outputStream are deprecated. ' + 'Use repl.input and repl.output instead', 'DEP0141') : () => this.output, set: pendingDeprecation ? deprecate((val) => this.output = val, - 'repl.inputStream and repl.outputStream is deprecated. ' + + 'repl.inputStream and repl.outputStream are deprecated. ' + 'Use repl.input and repl.output instead', 'DEP0141') : (val) => this.output = val, diff --git a/test/parallel/test-repl-options.js b/test/parallel/test-repl-options.js index 3f6e16ee47fba2..546dd584ea34fe 100644 --- a/test/parallel/test-repl-options.js +++ b/test/parallel/test-repl-options.js @@ -35,7 +35,7 @@ common.expectWarning({ DeprecationWarning: { DEP0142: 'repl._builtinLibs is deprecated. Check module.builtinModules instead', - DEP0141: 'repl.inputStream and repl.outputStream is deprecated. ' + + DEP0141: 'repl.inputStream and repl.outputStream are deprecated. ' + 'Use repl.input and repl.output instead', } }); From 9f0671ebc9fa564e59c79750f961f85b2d8fd026 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 4 Jul 2020 06:53:01 -0700 Subject: [PATCH 21/25] test: replace deprecated function call from test-repl-history-navigation test-repl-history-navigation fails with NODE_PENDING_DEPRECATION=1. Replace deprecated repl.inputStream with repl.input. PR-URL: https://github.com/nodejs/node/pull/34199 Reviewed-By: Anna Henningsen Reviewed-By: Ruben Bridgewater Reviewed-By: Trivikram Kamat Reviewed-By: James M Snell --- test/parallel/test-repl-history-navigation.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-repl-history-navigation.js b/test/parallel/test-repl-history-navigation.js index 9155ad722b8710..fa40ac624000f4 100644 --- a/test/parallel/test-repl-history-navigation.js +++ b/test/parallel/test-repl-history-navigation.js @@ -596,7 +596,7 @@ function runTest() { enumerable: true }); } - repl.inputStream.run(opts.test); + repl.input.run(opts.test); }); } From bf772896fe3621e6b2324c7dd610dfbd79f63ab9 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 4 Jul 2020 06:26:30 -0700 Subject: [PATCH 22/25] doc: remove errors that were never released Refs: https://github.com/nodejs/node/pull/33764#issuecomment-653667275 PR-URL: https://github.com/nodejs/node/pull/34197 Reviewed-By: Ruben Bridgewater Reviewed-By: Trivikram Kamat Reviewed-By: James M Snell --- doc/api/errors.md | 72 ----------------------------------------------- 1 file changed, 72 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index a33e31f224308e..e68155900869ef 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -2631,78 +2631,6 @@ removed: v10.0.0 Used when an attempt is made to use a `zlib` object after it has already been closed. -### Other error codes - -These errors have never been released, but had been present on master between -releases. - - -#### `ERR_ENTRY_TYPE_MISMATCH` - -The `--entry-type=commonjs` flag was used to attempt to execute an `.mjs` file -or a `.js` file where the nearest parent `package.json` contains -`"type": "module"`; or -the `--entry-type=module` flag was used to attempt to execute a `.cjs` file or -a `.js` file where the nearest parent `package.json` either lacks a `"type"` -field or contains `"type": "commonjs"`. - - -#### `ERR_FS_WATCHER_ALREADY_STARTED` - -An attempt was made to start a watcher returned by `fs.watch()` that has -already been started. - - -#### `ERR_FS_WATCHER_NOT_STARTED` - -An attempt was made to initiate operations on a watcher returned by -`fs.watch()` that has not yet been started. - - -#### `ERR_HTTP2_ALREADY_SHUTDOWN` - -Occurs with multiple attempts to shutdown an HTTP/2 session. - - -#### `ERR_HTTP2_ERROR` - -A non-specific HTTP/2 error has occurred. - - -#### `ERR_INVALID_REPL_HISTORY` - -Used in the `repl` in case the old history file is used and an error occurred -while trying to read and parse it. - - -#### `ERR_INVALID_REPL_TYPE` - -The `--entry-type=...` flag is not compatible with the Node.js REPL. - - -#### `ERR_STREAM_HAS_STRINGDECODER` - -Used to prevent an abort if a string decoder was set on the Socket. - -```js -const Socket = require('net').Socket; -const instance = new Socket(); - -instance.setEncoding('utf8'); -``` - - -#### `ERR_STRING_TOO_LARGE` - -An attempt has been made to create a string larger than the maximum allowed -size. - - -#### `ERR_TTY_WRITABLE_NOT_READABLE` - -This `Error` is thrown when a read is attempted on a TTY `WriteStream`, -such as `process.stdout.on('data')`. - [`'uncaughtException'`]: process.html#process_event_uncaughtexception [`--disable-proto=throw`]: cli.html#cli_disable_proto_mode [`--force-fips`]: cli.html#cli_force_fips From 3975799f260bb1aea77f3e8f4546182b53f2d6b8 Mon Sep 17 00:00:00 2001 From: sapics Date: Tue, 30 Jun 2020 00:16:11 +0900 Subject: [PATCH 23/25] doc: replace http to https of link urls PR-URL: https://github.com/nodejs/node/pull/34158 Reviewed-By: Anna Henningsen Reviewed-By: Anto Aravinth Reviewed-By: James M Snell --- doc/api/cli.md | 2 +- doc/api/dns.md | 4 ++-- doc/api/errors.md | 6 +++--- doc/api/esm.md | 8 ++++---- doc/api/fs.md | 2 +- doc/api/n-api.md | 4 ++-- doc/api/net.md | 2 +- doc/api/process.md | 2 +- doc/changelogs/CHANGELOG_ARCHIVE.md | 6 +++--- doc/changelogs/CHANGELOG_IOJS.md | 2 +- doc/guides/cpp-style-guide.md | 12 ++++++------ doc/guides/maintaining-icu.md | 4 ++-- doc/guides/maintaining-openssl.md | 2 +- doc/guides/writing-tests.md | 2 +- 14 files changed, 29 insertions(+), 29 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index c01edf1a1a1e8a..e546473ba3885e 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -1579,5 +1579,5 @@ $ node --max-old-space-size=1536 index.js [emit_warning]: process.html#process_process_emitwarning_warning_type_code_ctor [experimental ECMAScript Module loader]: esm.html#esm_experimental_loaders [jitless]: https://v8.dev/blog/jitless -[libuv threadpool documentation]: http://docs.libuv.org/en/latest/threadpool.html +[libuv threadpool documentation]: https://docs.libuv.org/en/latest/threadpool.html [remote code execution]: https://www.owasp.org/index.php/Code_Injection diff --git a/doc/api/dns.md b/doc/api/dns.md index 20554fdc1d75d3..73f3d80254ac2f 100644 --- a/doc/api/dns.md +++ b/doc/api/dns.md @@ -618,7 +618,7 @@ The [`dns.setServers()`][] method affects only [`dns.resolve()`][], [`dns.lookup()`][]). This method works much like -[resolve.conf](http://man7.org/linux/man-pages/man5/resolv.conf.5.html). +[resolve.conf](https://man7.org/linux/man-pages/man5/resolv.conf.5.html). That is, if attempting to resolve with the first server provided results in a `NOTFOUND` error, the `resolve()` method will *not* attempt to resolve with subsequent servers provided. Fallback DNS servers will only be used if the @@ -1082,7 +1082,7 @@ The `dnsPromises.setServers()` method must not be called while a DNS query is in progress. This method works much like -[resolve.conf](http://man7.org/linux/man-pages/man5/resolv.conf.5.html). +[resolve.conf](https://man7.org/linux/man-pages/man5/resolv.conf.5.html). That is, if attempting to resolve with the first server provided results in a `NOTFOUND` error, the `resolve()` method will *not* attempt to resolve with subsequent servers provided. Fallback DNS servers will only be used if the diff --git a/doc/api/errors.md b/doc/api/errors.md index e68155900869ef..92715e1236bb96 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -2654,7 +2654,7 @@ closed. [`dgram.createSocket()`]: dgram.html#dgram_dgram_createsocket_options_callback [`dgram.disconnect()`]: dgram.html#dgram_socket_disconnect [`dgram.remoteAddress()`]: dgram.html#dgram_socket_remoteaddress -[`errno`(3) man page]: http://man7.org/linux/man-pages/man3/errno.3.html +[`errno`(3) man page]: https://man7.org/linux/man-pages/man3/errno.3.html [`fs.Dir`]: fs.html#fs_class_fs_dir [`fs.readFileSync`]: fs.html#fs_fs_readfilesync_path_options [`fs.readdir`]: fs.html#fs_fs_readdir_path_options_callback @@ -2666,7 +2666,7 @@ closed. [`hash.update()`]: crypto.html#crypto_hash_update_data_inputencoding [`http`]: http.html [`https`]: https.html -[`libuv Error handling`]: http://docs.libuv.org/en/v1.x/errors.html +[`libuv Error handling`]: https://docs.libuv.org/en/v1.x/errors.html [`net`]: net.html [`new URL(input)`]: url.html#url_new_url_input_base [`new URLSearchParams(iterable)`]: url.html#url_new_urlsearchparams_iterable @@ -2702,7 +2702,7 @@ closed. [policy]: policy.html [RFC 7230 Section 3]: https://tools.ietf.org/html/rfc7230#section-3 [stream-based]: stream.html -[syscall]: http://man7.org/linux/man-pages/man2/syscalls.2.html +[syscall]: https://man7.org/linux/man-pages/man2/syscalls.2.html [Subresource Integrity specification]: https://www.w3.org/TR/SRI/#the-integrity-attribute [try-catch]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch [vm]: vm.html diff --git a/doc/api/esm.md b/doc/api/esm.md index c06ba4782af884..75a7c07ddbcbe3 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -1817,11 +1817,11 @@ success! [`module.createRequire()`]: modules.html#modules_module_createrequire_filename [`module.syncBuiltinESMExports()`]: modules.html#modules_module_syncbuiltinesmexports [`transformSource` hook]: #esm_code_transformsource_code_hook -[ArrayBuffer]: http://www.ecma-international.org/ecma-262/6.0/#sec-arraybuffer-constructor +[ArrayBuffer]: https://www.ecma-international.org/ecma-262/6.0/#sec-arraybuffer-constructor [SharedArrayBuffer]: https://tc39.es/ecma262/#sec-sharedarraybuffer-constructor -[string]: http://www.ecma-international.org/ecma-262/6.0/#sec-string-constructor -[TypedArray]: http://www.ecma-international.org/ecma-262/6.0/#sec-typedarray-objects -[Uint8Array]: http://www.ecma-international.org/ecma-262/6.0/#sec-uint8array +[string]: https://www.ecma-international.org/ecma-262/6.0/#sec-string-constructor +[TypedArray]: https://www.ecma-international.org/ecma-262/6.0/#sec-typedarray-objects +[Uint8Array]: https://www.ecma-international.org/ecma-262/6.0/#sec-uint8array [`util.TextDecoder`]: util.html#util_class_util_textdecoder [import an ES or CommonJS module for its side effects only]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#Import_a_module_for_its_side_effects_only [special scheme]: https://url.spec.whatwg.org/#special-scheme diff --git a/doc/api/fs.md b/doc/api/fs.md index 3332444aa4353d..d9d36b6e1024ea 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -5933,7 +5933,7 @@ the file contents. [`fsPromises.open()`]: #fs_fspromises_open_path_flags_mode [`fsPromises.opendir()`]: #fs_fspromises_opendir_path_options [`fsPromises.utimes()`]: #fs_fspromises_utimes_path_atime_mtime -[`inotify(7)`]: http://man7.org/linux/man-pages/man7/inotify.7.html +[`inotify(7)`]: https://man7.org/linux/man-pages/man7/inotify.7.html [`kqueue(2)`]: https://www.freebsd.org/cgi/man.cgi?query=kqueue&sektion=2 [`net.Socket`]: net.html#net_class_net_socket [`stat()`]: fs.html#fs_fs_stat_path_options_callback diff --git a/doc/api/n-api.md b/doc/api/n-api.md index b34563bfaac19e..10da5043e2fa14 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -5537,8 +5537,8 @@ This API may only be called from the main thread. [`napi_wrap`]: #n_api_napi_wrap [`node_api.h`]: https://github.com/nodejs/node/blob/master/src/node_api.h [`process.release`]: process.html#process_process_release -[`uv_ref`]: http://docs.libuv.org/en/v1.x/handle.html#c.uv_ref -[`uv_unref`]: http://docs.libuv.org/en/v1.x/handle.html#c.uv_unref +[`uv_ref`]: https://docs.libuv.org/en/v1.x/handle.html#c.uv_ref +[`uv_unref`]: https://docs.libuv.org/en/v1.x/handle.html#c.uv_unref [async_hooks `type`]: async_hooks.html#async_hooks_type [context-aware addons]: addons.html#addons_context_aware_addons [docs]: https://github.com/nodejs/node-addon-api#api-documentation diff --git a/doc/api/net.md b/doc/api/net.md index a07c34cee07835..3ad821a5af12b1 100644 --- a/doc/api/net.md +++ b/doc/api/net.md @@ -1259,7 +1259,7 @@ Returns `true` if input is a version 6 IP address, otherwise returns `false`. [`server.listen(handle)`]: #net_server_listen_handle_backlog_callback [`server.listen(options)`]: #net_server_listen_options_callback [`server.listen(path)`]: #net_server_listen_path_backlog_callback -[`socket(7)`]: http://man7.org/linux/man-pages/man7/socket.7.html +[`socket(7)`]: https://man7.org/linux/man-pages/man7/socket.7.html [`socket.connect()`]: #net_socket_connect [`socket.connect(options)`]: #net_socket_connect_options_connectlistener [`socket.connect(path)`]: #net_socket_connect_path_connectlistener diff --git a/doc/api/process.md b/doc/api/process.md index 7424b37f6e78e4..b10b0bf4a0aaed 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -2644,6 +2644,6 @@ cases: [process_warning]: #process_event_warning [report documentation]: report.html [terminal raw mode]: tty.html#tty_readstream_setrawmode_mode -[uv_rusage_t]: http://docs.libuv.org/en/v1.x/misc.html#c.uv_rusage_t +[uv_rusage_t]: https://docs.libuv.org/en/v1.x/misc.html#c.uv_rusage_t [wikipedia_minor_fault]: https://en.wikipedia.org/wiki/Page_fault#Minor [wikipedia_major_fault]: https://en.wikipedia.org/wiki/Page_fault#Major diff --git a/doc/changelogs/CHANGELOG_ARCHIVE.md b/doc/changelogs/CHANGELOG_ARCHIVE.md index 55302c99402b76..650d4c6c69cddd 100644 --- a/doc/changelogs/CHANGELOG_ARCHIVE.md +++ b/doc/changelogs/CHANGELOG_ARCHIVE.md @@ -3130,8 +3130,8 @@ https://github.com/nodejs/node/commit/bb0d1e65e1671aaeb21fac186b066701da0bc33b * Major API Changes * Promises removed. See - http://groups.google.com/group/nodejs/msg/426f3071f3eec16b - http://groups.google.com/group/nodejs/msg/df199d233ff17efa + https://groups.google.com/group/nodejs/msg/426f3071f3eec16b + https://groups.google.com/group/nodejs/msg/df199d233ff17efa The API for fs was fs.readdir("/usr").addCallback(function (files) { puts("/usr files: " + files); @@ -3718,7 +3718,7 @@ https://github.com/nodejs/node/commit/77d407df2826b20e9177c26c0d2bb4481e497937 * Move EventEmitter.prototype.emit() completely into C++. * Bugfix: Fix memory leak in event emitters. - http://groups.google.com/group/nodejs/browse_thread/thread/a8d1dfc2fd57a6d1 + https://groups.google.com/group/nodejs/browse_thread/thread/a8d1dfc2fd57a6d1 * Bugfix: Had problems reading scripts with non-ascii characters. * Bugfix: Fix Detach() in node::Server diff --git a/doc/changelogs/CHANGELOG_IOJS.md b/doc/changelogs/CHANGELOG_IOJS.md index 0a650253182b6d..540e244f1139a9 100644 --- a/doc/changelogs/CHANGELOG_IOJS.md +++ b/doc/changelogs/CHANGELOG_IOJS.md @@ -408,7 +408,7 @@ See https://github.com/nodejs/io.js/labels/confirmed-bug for complete and curren * **dgram**: If an error occurs within `socket.send()` and a callback has been provided, the error is only passed as the first argument to the callback and not emitted on the `socket` object; previous behavior was to do both (Matteo Collina & Chris Dickinson) [#1796](https://github.com/nodejs/node/pull/1796) * **freelist**: Deprecate the undocumented `freelist` core module (Sakthipriyan Vairamani) [#2176](https://github.com/nodejs/node/pull/2176). * **http**: - * Status codes now all use the official [IANA names](http://www.iana.org/assignments/http-status-codes) as per [RFC7231](https://tools.ietf.org/html/rfc7231), e.g. `http.STATUS_CODES[414]` now returns `'URI Too Long'` rather than `'Request-URI Too Large'` (jomo) [#1470](https://github.com/nodejs/node/pull/1470). + * Status codes now all use the official [IANA names](https://www.iana.org/assignments/http-status-codes) as per [RFC7231](https://tools.ietf.org/html/rfc7231), e.g. `http.STATUS_CODES[414]` now returns `'URI Too Long'` rather than `'Request-URI Too Large'` (jomo) [#1470](https://github.com/nodejs/node/pull/1470). * Calling .getName() on an HTTP agent no longer returns a trailing colon, HTTPS agents will no longer return an extra colon near the middle of the string (Brendan Ashworth) [#1617](https://github.com/nodejs/node/pull/1617). * **node**: * `NODE_MODULE_VERSION` has been bumped to `45` to reflect the break in ABI (Rod Vagg) [#2096](https://github.com/nodejs/node/pull/2096). diff --git a/doc/guides/cpp-style-guide.md b/doc/guides/cpp-style-guide.md index 85db8e1595aa9c..adf8f0481697f8 100644 --- a/doc/guides/cpp-style-guide.md +++ b/doc/guides/cpp-style-guide.md @@ -391,15 +391,15 @@ side effects. Node.js is built [without C++ exception handling][], so code using `throw` or even `try` and `catch` **will** break. -[C++ Core Guidelines]: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines +[C++ Core Guidelines]: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines [Google C++ Style Guide]: https://google.github.io/styleguide/cppguide.html [Google’s `cpplint`]: https://github.com/google/styleguide [errors]: https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md -[ES.47]: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-nullptr -[ES.48]: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts -[ES.49]: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts-named -[R.20]: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-owner -[R.21]: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-unique +[ES.47]: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-nullptr +[ES.48]: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts +[ES.49]: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts-named +[R.20]: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-owner +[R.21]: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-unique [Run Time Type Information]: https://en.wikipedia.org/wiki/Run-time_type_information [cppref_auto_ptr]: https://en.cppreference.com/w/cpp/memory/auto_ptr [without C++ exception handling]: https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_exceptions.html#intro.using.exception.no diff --git a/doc/guides/maintaining-icu.md b/doc/guides/maintaining-icu.md index 083c11ba938481..bf5727d4e865c3 100644 --- a/doc/guides/maintaining-icu.md +++ b/doc/guides/maintaining-icu.md @@ -40,7 +40,7 @@ main data files do not need to be upgraded in order to apply time zone data file fixes. The [IANA tzdata](https://www.iana.org/time-zones) project releases new versions -and announces them on the [`tz-announce`](http://mm.icann.org/pipermail/tz-announce/) +and announces them on the [`tz-announce`](https://mm.icann.org/pipermail/tz-announce/) mailing list. The Unicode project takes new releases and publishes @@ -98,7 +98,7 @@ Node.js is built. * Make sure your Node.js workspace is clean (`git status` should be sufficient). -* Configure Node.js with the specific [ICU version](http://icu-project.org/download) +* Configure Node.js with the specific [ICU version](http://site.icu-project.org/download) you want to upgrade to, for example: ```bash diff --git a/doc/guides/maintaining-openssl.md b/doc/guides/maintaining-openssl.md index 4c391d584c5f55..603d82a0ae23ea 100644 --- a/doc/guides/maintaining-openssl.md +++ b/doc/guides/maintaining-openssl.md @@ -5,7 +5,7 @@ This document describes how to update `deps/openssl/`. ## Requirements * Linux environment. * `perl` Only Perl version 5 is tested. -* `nasm` () Version 2.11 or higher is needed. +* `nasm` () Version 2.11 or higher is needed. * GNU `as` in binutils. Version 2.26 or higher is needed. ## 0. Check Requirements diff --git a/doc/guides/writing-tests.md b/doc/guides/writing-tests.md index e441d934113e75..0aa4cb8221f278 100644 --- a/doc/guides/writing-tests.md +++ b/doc/guides/writing-tests.md @@ -432,7 +432,7 @@ To generate a test coverage report, see the Nightly coverage reports for the Node.js master branch are available at . -[ASCII]: http://man7.org/linux/man-pages/man7/ascii.7.html +[ASCII]: https://man7.org/linux/man-pages/man7/ascii.7.html [Google Test]: https://github.com/google/googletest [`common` module]: https://github.com/nodejs/node/blob/master/test/common/README.md [all maintained branches]: https://github.com/nodejs/lts From ee3416b055a3d877251c095981508e275d99f8c2 Mon Sep 17 00:00:00 2001 From: sapics Date: Mon, 29 Jun 2020 23:58:19 +0900 Subject: [PATCH 24/25] lib: replace http to https of comment link urls PR-URL: https://github.com/nodejs/node/pull/34158 Reviewed-By: Anna Henningsen Reviewed-By: Anto Aravinth Reviewed-By: James M Snell --- lib/_http_server.js | 2 +- lib/https.js | 2 +- lib/internal/child_process.js | 2 +- lib/internal/source_map/source_map.js | 3 ++- lib/internal/tty.js | 2 +- lib/internal/util/inspect.js | 4 ++-- lib/querystring.js | 2 +- lib/readline.js | 2 +- 8 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 97107cead89d5c..45622eab5be3eb 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -361,7 +361,7 @@ function Server(options, requestListener) { // Similar option to this. Too lazy to write my own docs. // http://www.squid-cache.org/Doc/config/half_closed_clients/ - // http://wiki.squid-cache.org/SquidFaq/InnerWorkings#What_is_a_half-closed_filedescriptor.3F + // https://wiki.squid-cache.org/SquidFaq/InnerWorkings#What_is_a_half-closed_filedescriptor.3F this.httpAllowHalfOpen = false; this.on('connection', connectionListener); diff --git a/lib/https.js b/lib/https.js index 1d3a6e5024b65b..17a89c2f0bc0bb 100644 --- a/lib/https.js +++ b/lib/https.js @@ -55,7 +55,7 @@ function Server(opts, requestListener) { if (!opts.ALPNProtocols) { // http/1.0 is not defined as Protocol IDs in IANA - // http://www.iana.org/assignments/tls-extensiontype-values + // https://www.iana.org/assignments/tls-extensiontype-values // /tls-extensiontype-values.xhtml#alpn-protocol-ids opts.ALPNProtocols = ['http/1.1']; } diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 6438e6b650b814..85ce63a9660f86 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -944,7 +944,7 @@ function getValidStdio(stdio, sync) { // At least 3 stdio will be created // Don't concat() a new Array() because it would be sparse, and // stdio.reduce() would skip the sparse elements of stdio. - // See http://stackoverflow.com/a/5501711/3561 + // See https://stackoverflow.com/a/5501711/3561 while (stdio.length < 3) stdio.push(undefined); // Translate stdio into C++-readable form diff --git a/lib/internal/source_map/source_map.js b/lib/internal/source_map/source_map.js index 2cd70c5c944e19..7d21e02429a086 100644 --- a/lib/internal/source_map/source_map.js +++ b/lib/internal/source_map/source_map.js @@ -113,7 +113,8 @@ class StringCharIterator { } /** - * Implements Source Map V3 model. See http://code.google.com/p/closure-compiler/wiki/SourceMaps + * Implements Source Map V3 model. + * See https://github.com/google/closure-compiler/wiki/Source-Maps * for format description. * @constructor * @param {string} sourceMappingURL diff --git a/lib/internal/tty.js b/lib/internal/tty.js index 44d51737008eb0..1ebd09193ef57e 100644 --- a/lib/internal/tty.js +++ b/lib/internal/tty.js @@ -125,7 +125,7 @@ function getColorDepth(env = process.env) { env.NO_COLOR !== undefined || // The "dumb" special terminal, as defined by terminfo, doesn't support // ANSI color control codes. - // See http://invisible-island.net/ncurses/terminfo.ti.html#toc-_Specials + // See https://invisible-island.net/ncurses/terminfo.ti.html#toc-_Specials env.TERM === 'dumb') { return COLORS_2; } diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js index 8cab2079367145..7502a21a947df7 100644 --- a/lib/internal/util/inspect.js +++ b/lib/internal/util/inspect.js @@ -338,7 +338,7 @@ ObjectDefineProperty(inspect, 'defaultOptions', { } }); -// Set Graphics Rendition http://en.wikipedia.org/wiki/ANSI_escape_code#graphics +// Set Graphics Rendition https://en.wikipedia.org/wiki/ANSI_escape_code#graphics // Each color consists of an array with the color code as first entry and the // reset code as second entry. const defaultFG = 39; @@ -2051,7 +2051,7 @@ if (internalBinding('config').hasIntl) { */ const isFullWidthCodePoint = (code) => { // Code points are partially derived from: - // http://www.unicode.org/Public/UNIDATA/EastAsianWidth.txt + // https://www.unicode.org/Public/UNIDATA/EastAsianWidth.txt return code >= 0x1100 && ( code <= 0x115f || // Hangul Jamo code === 0x2329 || // LEFT-POINTING ANGLE BRACKET diff --git a/lib/querystring.js b/lib/querystring.js index f057226f08b51f..05cca60fc4c57f 100644 --- a/lib/querystring.js +++ b/lib/querystring.js @@ -140,7 +140,7 @@ const noEscape = [ 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 1, 0 // 112 - 127 ]; // QueryString.escape() replaces encodeURIComponent() -// http://www.ecma-international.org/ecma-262/5.1/#sec-15.1.3.4 +// https://www.ecma-international.org/ecma-262/5.1/#sec-15.1.3.4 function qsEscape(str) { if (typeof str !== 'string') { if (typeof str === 'object') diff --git a/lib/readline.js b/lib/readline.js index 7d44dd2366cf39..a8f60ac9111e26 100644 --- a/lib/readline.js +++ b/lib/readline.js @@ -22,7 +22,7 @@ // Inspiration for this code comes from Salvatore Sanfilippo's linenoise. // https://github.com/antirez/linenoise // Reference: -// * http://invisible-island.net/xterm/ctlseqs/ctlseqs.html +// * https://invisible-island.net/xterm/ctlseqs/ctlseqs.html // * http://www.3waylabs.com/nw/WWW/products/wizcon/vt220.html 'use strict'; From 772fdb0cd3163ad9299cdb1168b10059abe6ee71 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 4 Jul 2020 09:37:02 -0700 Subject: [PATCH 25/25] test: fix flaky test-fs-stream-construct The test is marked flaky on ARM because it times out on Raspberry Pi devices in CI. Split the single test file into four separate test files to ease debugging. Add fs.close() to avoid timing out. Fixes: https://github.com/nodejs/node/issues/33796 PR-URL: https://github.com/nodejs/node/pull/34203 Reviewed-By: Anna Henningsen Reviewed-By: Robert Nagy --- test/parallel/parallel.status | 2 - ...t-fs-stream-construct-compat-error-read.js | 32 +++ ...-fs-stream-construct-compat-error-write.js | 50 +++++ ...-fs-stream-construct-compat-graceful-fs.js | 70 ++++++ ...est-fs-stream-construct-compat-old-node.js | 97 ++++++++ test/parallel/test-fs-stream-construct.js | 212 ------------------ 6 files changed, 249 insertions(+), 214 deletions(-) create mode 100644 test/parallel/test-fs-stream-construct-compat-error-read.js create mode 100644 test/parallel/test-fs-stream-construct-compat-error-write.js create mode 100644 test/parallel/test-fs-stream-construct-compat-graceful-fs.js create mode 100644 test/parallel/test-fs-stream-construct-compat-old-node.js delete mode 100644 test/parallel/test-fs-stream-construct.js diff --git a/test/parallel/parallel.status b/test/parallel/parallel.status index 94d15064b38423..c6fd6fc178b510 100644 --- a/test/parallel/parallel.status +++ b/test/parallel/parallel.status @@ -40,8 +40,6 @@ test-async-hooks-http-parser-destroy: PASS,FLAKY # https://github.com/nodejs/node/pull/31178 test-crypto-dh-stateless: SKIP test-crypto-keygen: SKIP -# https://github.com/nodejs/node/issues/33796 -test-fs-stream-construct: PASS,FLAKY [$system==solaris] # Also applies to SmartOS diff --git a/test/parallel/test-fs-stream-construct-compat-error-read.js b/test/parallel/test-fs-stream-construct-compat-error-read.js new file mode 100644 index 00000000000000..0b7297a59f1bd7 --- /dev/null +++ b/test/parallel/test-fs-stream-construct-compat-error-read.js @@ -0,0 +1,32 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const assert = require('assert'); + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +{ + // Compat error. + + function ReadStream(...args) { + fs.ReadStream.call(this, ...args); + } + Object.setPrototypeOf(ReadStream.prototype, fs.ReadStream.prototype); + Object.setPrototypeOf(ReadStream, fs.ReadStream); + + ReadStream.prototype.open = common.mustCall(function ReadStream$open() { + const that = this; + fs.open(that.path, that.flags, that.mode, (err, fd) => { + that.emit('error', err); + }); + }); + + const r = new ReadStream('/doesnotexist', { emitClose: true }) + .on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ENOENT'); + assert.strictEqual(r.destroyed, true); + r.on('close', common.mustCall()); + })); +} diff --git a/test/parallel/test-fs-stream-construct-compat-error-write.js b/test/parallel/test-fs-stream-construct-compat-error-write.js new file mode 100644 index 00000000000000..b47632c2c95e2f --- /dev/null +++ b/test/parallel/test-fs-stream-construct-compat-error-write.js @@ -0,0 +1,50 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const assert = require('assert'); + +const debuglog = (arg) => { + console.log(new Date().toLocaleString(), arg); +}; + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +{ + // Compat error. + debuglog('start test'); + + function WriteStream(...args) { + debuglog('WriteStream constructor'); + fs.WriteStream.call(this, ...args); + } + Object.setPrototypeOf(WriteStream.prototype, fs.WriteStream.prototype); + Object.setPrototypeOf(WriteStream, fs.WriteStream); + + WriteStream.prototype.open = common.mustCall(function WriteStream$open() { + debuglog('WriteStream open() callback'); + const that = this; + fs.open(that.path, that.flags, that.mode, (err, fd) => { + debuglog('inner fs open() callback'); + that.emit('error', err); + }); + }); + + fs.open(`${tmpdir.path}/dummy`, 'wx+', common.mustCall((err, fd) => { + debuglog('fs open() callback'); + assert.ifError(err); + fs.close(fd, () => { debuglog(`closed ${fd}`); }); + const w = new WriteStream(`${tmpdir.path}/dummy`, + { flags: 'wx+', emitClose: true }) + .on('error', common.mustCall((err) => { + debuglog('error event callback'); + assert.strictEqual(err.code, 'EEXIST'); + w.destroy(); + w.on('close', common.mustCall(() => { + debuglog('close event callback'); + })); + })); + })); + debuglog('waiting for callbacks'); +} diff --git a/test/parallel/test-fs-stream-construct-compat-graceful-fs.js b/test/parallel/test-fs-stream-construct-compat-graceful-fs.js new file mode 100644 index 00000000000000..ee1e00ed676042 --- /dev/null +++ b/test/parallel/test-fs-stream-construct-compat-graceful-fs.js @@ -0,0 +1,70 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const assert = require('assert'); +const fixtures = require('../common/fixtures'); + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +{ + // Compat with graceful-fs. + + function ReadStream(...args) { + fs.ReadStream.call(this, ...args); + } + Object.setPrototypeOf(ReadStream.prototype, fs.ReadStream.prototype); + Object.setPrototypeOf(ReadStream, fs.ReadStream); + + ReadStream.prototype.open = common.mustCall(function ReadStream$open() { + const that = this; + fs.open(that.path, that.flags, that.mode, (err, fd) => { + if (err) { + if (that.autoClose) + that.destroy(); + + that.emit('error', err); + } else { + that.fd = fd; + that.emit('open', fd); + that.read(); + } + }); + }); + + const r = new ReadStream(fixtures.path('x.txt')) + .on('open', common.mustCall((fd) => { + assert.strictEqual(fd, r.fd); + r.destroy(); + })); +} + +{ + // Compat with graceful-fs. + + function WriteStream(...args) { + fs.WriteStream.call(this, ...args); + } + Object.setPrototypeOf(WriteStream.prototype, fs.WriteStream.prototype); + Object.setPrototypeOf(WriteStream, fs.WriteStream); + + WriteStream.prototype.open = common.mustCall(function WriteStream$open() { + const that = this; + fs.open(that.path, that.flags, that.mode, function(err, fd) { + if (err) { + that.destroy(); + that.emit('error', err); + } else { + that.fd = fd; + that.emit('open', fd); + } + }); + }); + + const w = new WriteStream(`${tmpdir.path}/dummy`) + .on('open', common.mustCall((fd) => { + assert.strictEqual(fd, w.fd); + w.destroy(); + })); +} diff --git a/test/parallel/test-fs-stream-construct-compat-old-node.js b/test/parallel/test-fs-stream-construct-compat-old-node.js new file mode 100644 index 00000000000000..bd5aec689ff3b7 --- /dev/null +++ b/test/parallel/test-fs-stream-construct-compat-old-node.js @@ -0,0 +1,97 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const assert = require('assert'); +const fixtures = require('../common/fixtures'); + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +{ + // Compat with old node. + + function ReadStream(...args) { + fs.ReadStream.call(this, ...args); + } + Object.setPrototypeOf(ReadStream.prototype, fs.ReadStream.prototype); + Object.setPrototypeOf(ReadStream, fs.ReadStream); + + ReadStream.prototype.open = common.mustCall(function() { + fs.open(this.path, this.flags, this.mode, (er, fd) => { + if (er) { + if (this.autoClose) { + this.destroy(); + } + this.emit('error', er); + return; + } + + this.fd = fd; + this.emit('open', fd); + this.emit('ready'); + }); + }); + + let readyCalled = false; + let ticked = false; + const r = new ReadStream(fixtures.path('x.txt')) + .on('ready', common.mustCall(() => { + readyCalled = true; + // Make sure 'ready' is emitted in same tick as 'open'. + assert.strictEqual(ticked, false); + })) + .on('error', common.mustNotCall()) + .on('open', common.mustCall((fd) => { + process.nextTick(() => { + ticked = true; + r.destroy(); + }); + assert.strictEqual(readyCalled, false); + assert.strictEqual(fd, r.fd); + })); +} + +{ + // Compat with old node. + + function WriteStream(...args) { + fs.WriteStream.call(this, ...args); + } + Object.setPrototypeOf(WriteStream.prototype, fs.WriteStream.prototype); + Object.setPrototypeOf(WriteStream, fs.WriteStream); + + WriteStream.prototype.open = common.mustCall(function() { + fs.open(this.path, this.flags, this.mode, (er, fd) => { + if (er) { + if (this.autoClose) { + this.destroy(); + } + this.emit('error', er); + return; + } + + this.fd = fd; + this.emit('open', fd); + this.emit('ready'); + }); + }); + + let readyCalled = false; + let ticked = false; + const w = new WriteStream(`${tmpdir.path}/dummy`) + .on('ready', common.mustCall(() => { + readyCalled = true; + // Make sure 'ready' is emitted in same tick as 'open'. + assert.strictEqual(ticked, false); + })) + .on('error', common.mustNotCall()) + .on('open', common.mustCall((fd) => { + process.nextTick(() => { + ticked = true; + w.destroy(); + }); + assert.strictEqual(readyCalled, false); + assert.strictEqual(fd, w.fd); + })); +} diff --git a/test/parallel/test-fs-stream-construct.js b/test/parallel/test-fs-stream-construct.js deleted file mode 100644 index 4e687838f6b0c8..00000000000000 --- a/test/parallel/test-fs-stream-construct.js +++ /dev/null @@ -1,212 +0,0 @@ -'use strict'; - -const common = require('../common'); -const fs = require('fs'); -const assert = require('assert'); -const fixtures = require('../common/fixtures'); - -const tmpdir = require('../common/tmpdir'); -tmpdir.refresh(); - -const examplePath = fixtures.path('x.txt'); - -{ - // Compat with old node. - - function ReadStream(...args) { - fs.ReadStream.call(this, ...args); - } - Object.setPrototypeOf(ReadStream.prototype, fs.ReadStream.prototype); - Object.setPrototypeOf(ReadStream, fs.ReadStream); - - ReadStream.prototype.open = common.mustCall(function() { - fs.open(this.path, this.flags, this.mode, (er, fd) => { - if (er) { - if (this.autoClose) { - this.destroy(); - } - this.emit('error', er); - return; - } - - this.fd = fd; - this.emit('open', fd); - this.emit('ready'); - }); - }); - - let readyCalled = false; - let ticked = false; - const r = new ReadStream(examplePath) - .on('ready', common.mustCall(() => { - readyCalled = true; - // Make sure 'ready' is emitted in same tick as 'open'. - assert.strictEqual(ticked, false); - })) - .on('error', common.mustNotCall()) - .on('open', common.mustCall((fd) => { - process.nextTick(() => { - ticked = true; - r.destroy(); - }); - assert.strictEqual(readyCalled, false); - assert.strictEqual(fd, r.fd); - })); -} - -{ - // Compat with old node. - - function WriteStream(...args) { - fs.WriteStream.call(this, ...args); - } - Object.setPrototypeOf(WriteStream.prototype, fs.WriteStream.prototype); - Object.setPrototypeOf(WriteStream, fs.WriteStream); - - WriteStream.prototype.open = common.mustCall(function() { - fs.open(this.path, this.flags, this.mode, (er, fd) => { - if (er) { - if (this.autoClose) { - this.destroy(); - } - this.emit('error', er); - return; - } - - this.fd = fd; - this.emit('open', fd); - this.emit('ready'); - }); - }); - - let readyCalled = false; - let ticked = false; - const w = new WriteStream(`${tmpdir.path}/dummy`) - .on('ready', common.mustCall(() => { - readyCalled = true; - // Make sure 'ready' is emitted in same tick as 'open'. - assert.strictEqual(ticked, false); - })) - .on('error', common.mustNotCall()) - .on('open', common.mustCall((fd) => { - process.nextTick(() => { - ticked = true; - w.destroy(); - }); - assert.strictEqual(readyCalled, false); - assert.strictEqual(fd, w.fd); - })); -} - -{ - // Compat with graceful-fs. - - function ReadStream(...args) { - fs.ReadStream.call(this, ...args); - } - Object.setPrototypeOf(ReadStream.prototype, fs.ReadStream.prototype); - Object.setPrototypeOf(ReadStream, fs.ReadStream); - - ReadStream.prototype.open = common.mustCall(function ReadStream$open() { - const that = this; - fs.open(that.path, that.flags, that.mode, (err, fd) => { - if (err) { - if (that.autoClose) - that.destroy(); - - that.emit('error', err); - } else { - that.fd = fd; - that.emit('open', fd); - that.read(); - } - }); - }); - - const r = new ReadStream(examplePath) - .on('open', common.mustCall((fd) => { - assert.strictEqual(fd, r.fd); - r.destroy(); - })); -} - -{ - // Compat with graceful-fs. - - function WriteStream(...args) { - fs.WriteStream.call(this, ...args); - } - Object.setPrototypeOf(WriteStream.prototype, fs.WriteStream.prototype); - Object.setPrototypeOf(WriteStream, fs.WriteStream); - - WriteStream.prototype.open = common.mustCall(function WriteStream$open() { - const that = this; - fs.open(that.path, that.flags, that.mode, function(err, fd) { - if (err) { - that.destroy(); - that.emit('error', err); - } else { - that.fd = fd; - that.emit('open', fd); - } - }); - }); - - const w = new WriteStream(`${tmpdir.path}/dummy2`) - .on('open', common.mustCall((fd) => { - assert.strictEqual(fd, w.fd); - w.destroy(); - })); -} - -{ - // Compat error. - - function ReadStream(...args) { - fs.ReadStream.call(this, ...args); - } - Object.setPrototypeOf(ReadStream.prototype, fs.ReadStream.prototype); - Object.setPrototypeOf(ReadStream, fs.ReadStream); - - ReadStream.prototype.open = common.mustCall(function ReadStream$open() { - const that = this; - fs.open(that.path, that.flags, that.mode, (err, fd) => { - that.emit('error', err); - }); - }); - - const r = new ReadStream('/doesnotexist', { emitClose: true }) - .on('error', common.mustCall((err) => { - assert.strictEqual(err.code, 'ENOENT'); - assert.strictEqual(r.destroyed, true); - r.on('close', common.mustCall()); - })); -} - -{ - // Compat error. - - function WriteStream(...args) { - fs.WriteStream.call(this, ...args); - } - Object.setPrototypeOf(WriteStream.prototype, fs.WriteStream.prototype); - Object.setPrototypeOf(WriteStream, fs.WriteStream); - - WriteStream.prototype.open = common.mustCall(function WriteStream$open() { - const that = this; - fs.open(that.path, that.flags, that.mode, (err, fd) => { - that.emit('error', err); - }); - }); - - fs.open(`${tmpdir.path}/dummy3`, 'wx+', common.mustCall((err) => { - assert(!err); - const w = new WriteStream(`${tmpdir.path}/dummy3`, - { flags: 'wx+', emitClose: true }) - .on('error', common.mustCall((err) => { - assert.strictEqual(err.code, 'EEXIST'); - w.destroy(); - w.on('close', common.mustCall()); - })); - })); -}