From 560466c7738c1b66f2a74381bff41bdd7bde1eef Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 8 Mar 2019 09:47:45 +0100 Subject: [PATCH] lib,src: remove usage of _externalStream Since 4697e1b0d792f50863bbbcad25a95b84e6746501, it is no longer necessary to use `v8::External`s to pass `StreamBase` instances to native functions. PR-URL: https://github.com/nodejs/node/pull/26510 Refs: https://github.com/nodejs/node/pull/25142 Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell --- lib/_http_server.js | 12 +++++------- lib/_tls_wrap.js | 8 ++------ lib/internal/http2/core.js | 7 +++---- src/node_http2.cc | 8 ++++---- src/node_http2.h | 2 +- src/node_http_parser_impl.h | 5 ++--- src/stream_base-inl.h | 1 - src/stream_base.cc | 4 ++++ src/stream_pipe.cc | 9 ++++----- src/tls_wrap.cc | 3 +-- test/sequential/test-async-wrap-getasyncid.js | 3 +-- test/sequential/test-http-regr-gh-2928.js | 4 ++-- 12 files changed, 29 insertions(+), 37 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index a492a3f6523a2d..44af558b55f063 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -390,13 +390,11 @@ function connectionListenerInternal(server, socket) { socket.on = socketOnWrap; // We only consume the socket if it has never been consumed before. - if (socket._handle) { - var external = socket._handle._externalStream; - if (!socket._handle._consumed && external) { - parser._consumed = true; - socket._handle._consumed = true; - parser.consume(external); - } + if (socket._handle && socket._handle.isStreamBase && + !socket._handle._consumed) { + parser._consumed = true; + socket._handle._consumed = true; + parser.consume(socket._handle); } parser[kOnExecute] = onParserExecute.bind(undefined, server, socket, parser, state); diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 432d21ba4b309d..9f6db7c8de88fd 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -444,14 +444,10 @@ TLSSocket.prototype._wrapHandle = function(wrap) { const context = options.secureContext || options.credentials || tls.createSecureContext(options); - const externalStream = handle._externalStream; - assert(typeof externalStream === 'object', - 'handle must be a LibuvStreamWrap'); + assert(handle.isStreamBase, 'handle must be a StreamBase'); assert(context.context instanceof NativeSecureContext, 'context.context must be a NativeSecureContext'); - const res = tls_wrap.wrap(externalStream, - context.context, - !!options.isServer); + const res = tls_wrap.wrap(handle, context.context, !!options.isServer); res._parent = handle; // C++ "wrap" object: TCPWrap, JSStream, ... res._parentWrap = wrap; // JS object: net.Socket, JSStreamSocket, ... res._secureContext = context; diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index dd287ed5eb0d3f..4893cdc96b0b9b 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -843,7 +843,7 @@ function setupHandle(socket, type, options) { if (typeof options.selectPadding === 'function') this[kSelectPadding] = options.selectPadding; - handle.consume(socket._handle._externalStream); + handle.consume(socket._handle); this[kHandle] = handle; @@ -933,7 +933,7 @@ class Http2Session extends EventEmitter { constructor(type, options, socket) { super(); - if (!socket._handle || !socket._handle._externalStream) { + if (!socket._handle || !socket._handle.isStreamBase) { socket = new JSStreamSocket(socket); } @@ -2092,8 +2092,7 @@ function startFilePipe(self, fd, offset, length) { handle.onread = onPipedFileHandleRead; handle.stream = self; - const pipe = new StreamPipe(handle._externalStream, - self[kHandle]._externalStream); + const pipe = new StreamPipe(handle, self[kHandle]); pipe.onunpipe = onFileUnpipe; pipe.start(); diff --git a/src/node_http2.cc b/src/node_http2.cc index 62e66620dbb9f2..cbf35194881e43 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1837,8 +1837,8 @@ bool Http2Session::HasWritesOnSocketForStream(Http2Stream* stream) { // (typically a net.Socket or tls.TLSSocket). The lifecycle of the two is // tightly coupled with all data transfer between the two happening at the // C++ layer via the StreamBase API. -void Http2Session::Consume(Local external) { - StreamBase* stream = static_cast(external->Value()); +void Http2Session::Consume(Local stream_obj) { + StreamBase* stream = StreamBase::FromObject(stream_obj); stream->PushStreamListener(this); Debug(this, "i/o stream consumed"); } @@ -2429,8 +2429,8 @@ void Http2Session::New(const FunctionCallbackInfo& args) { void Http2Session::Consume(const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - CHECK(args[0]->IsExternal()); - session->Consume(args[0].As()); + CHECK(args[0]->IsObject()); + session->Consume(args[0].As()); } // Destroys the Http2Session instance and renders it unusable diff --git a/src/node_http2.h b/src/node_http2.h index c65da807d0cee0..c5c0ebcc2b0c66 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -696,7 +696,7 @@ class Http2Session : public AsyncWrap, public StreamListener { void Close(uint32_t code = NGHTTP2_NO_ERROR, bool socket_closed = false); - void Consume(Local external); + void Consume(Local stream); void Goaway(uint32_t code, int32_t lastStreamID, const uint8_t* data, size_t len); void AltSvc(int32_t id, diff --git a/src/node_http_parser_impl.h b/src/node_http_parser_impl.h index ae780d5371dc86..7a51a3140d212e 100644 --- a/src/node_http_parser_impl.h +++ b/src/node_http_parser_impl.h @@ -556,9 +556,8 @@ class Parser : public AsyncWrap, public StreamListener { static void Consume(const FunctionCallbackInfo& args) { Parser* parser; ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); - CHECK(args[0]->IsExternal()); - Local stream_obj = args[0].As(); - StreamBase* stream = static_cast(stream_obj->Value()); + CHECK(args[0]->IsObject()); + StreamBase* stream = StreamBase::FromObject(args[0].As()); CHECK_NOT_NULL(stream); stream->PushStreamListener(parser); } diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 368dcafe1c3291..dbf3c07b920cf8 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -12,7 +12,6 @@ namespace node { using v8::Signature; -using v8::External; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; diff --git a/src/stream_base.cc b/src/stream_base.cc index bf7003e12764e7..8eccca1a3e87a1 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -17,6 +17,7 @@ namespace node { using v8::Array; using v8::ArrayBuffer; using v8::Context; +using v8::External; using v8::FunctionCallbackInfo; using v8::HandleScope; using v8::Integer; @@ -368,6 +369,9 @@ void StreamBase::AddMethods(Environment* env, Local t) { t, "writeUcs2String", JSMethod<&StreamBase::WriteString>); env->SetProtoMethod( t, "writeLatin1String", JSMethod<&StreamBase::WriteString>); + t->PrototypeTemplate()->Set(FIXED_ONE_BYTE_STRING(env->isolate(), + "isStreamBase"), + True(env->isolate())); } void StreamBase::GetFD(const FunctionCallbackInfo& args) { diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc index 1a9d34b133bf0c..a57ce274fad57a 100644 --- a/src/stream_pipe.cc +++ b/src/stream_pipe.cc @@ -3,7 +3,6 @@ #include "node_buffer.h" using v8::Context; -using v8::External; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -226,10 +225,10 @@ void StreamPipe::WritableListener::OnStreamRead(ssize_t nread, void StreamPipe::New(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); - CHECK(args[0]->IsExternal()); - CHECK(args[1]->IsExternal()); - auto source = static_cast(args[0].As()->Value()); - auto sink = static_cast(args[1].As()->Value()); + CHECK(args[0]->IsObject()); + CHECK(args[1]->IsObject()); + StreamBase* source = StreamBase::FromObject(args[0].As()); + StreamBase* sink = StreamBase::FromObject(args[1].As()); new StreamPipe(source, sink, args.This()); } diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 84fc6e299ee860..a0eb51e6eea13d 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -151,12 +151,11 @@ void TLSWrap::Wrap(const FunctionCallbackInfo& args) { CHECK(args[1]->IsObject()); CHECK(args[2]->IsBoolean()); - Local stream_obj = args[0].As(); Local sc = args[1].As(); Kind kind = args[2]->IsTrue() ? SSLWrap::kServer : SSLWrap::kClient; - StreamBase* stream = static_cast(stream_obj->Value()); + StreamBase* stream = StreamBase::FromObject(args[0].As()); CHECK_NOT_NULL(stream); Local obj; diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js index a5e2bf64d82fc2..65f6f8e703131b 100644 --- a/test/sequential/test-async-wrap-getasyncid.js +++ b/test/sequential/test-async-wrap-getasyncid.js @@ -268,8 +268,7 @@ if (common.hasCrypto) { // eslint-disable-line node-core/crypto-check // TLSWrap is exposed, but needs to be instantiated via tls_wrap.wrap(). const tls_wrap = internalBinding('tls_wrap'); - testInitialized( - tls_wrap.wrap(tcp._externalStream, credentials.context, true), 'TLSWrap'); + testInitialized(tls_wrap.wrap(tcp, credentials.context, true), 'TLSWrap'); } { diff --git a/test/sequential/test-http-regr-gh-2928.js b/test/sequential/test-http-regr-gh-2928.js index aa2e2bc9acb6aa..094193f4574af0 100644 --- a/test/sequential/test-http-regr-gh-2928.js +++ b/test/sequential/test-http-regr-gh-2928.js @@ -40,12 +40,12 @@ function execAndClose() { throw e; }); - parser.consume(socket._handle._externalStream); + parser.consume(socket._handle); parser.onIncoming = function onIncoming() { process.stdout.write('+'); gotResponses++; - parser.unconsume(socket._handle._externalStream); + parser.unconsume(); httpCommon.freeParser(parser); socket.destroy(); setImmediate(execAndClose);