lib,src: remove usage of _externalStream

Since 4697e1b, it is no longer necessary to use `v8::External`s to pass `StreamBase` instances to native functions. PR-URL: #26510 Refs: #25142 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Mar 11, 2019 · 560466c · 560466c
1 parent a445244
commit 560466c
Show file tree

Hide file tree

Showing 12 changed files with 29 additions and 37 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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> external) {
-  StreamBase* stream = static_cast<StreamBase*>(external->Value());
+void Http2Session::Consume(Local<Object> 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<Value>& args) {
 void Http2Session::Consume(const FunctionCallbackInfo<Value>& args) {
   Http2Session* session;
   ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
-  CHECK(args[0]->IsExternal());
-  session->Consume(args[0].As<External>());
+  CHECK(args[0]->IsObject());
+  session->Consume(args[0].As<Object>());
 }
 
 // Destroys the Http2Session instance and renders it unusable

diff --git 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> external);
+  void Consume(Local<Object> 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
@@ -556,9 +556,8 @@ class Parser : public AsyncWrap, public StreamListener {
   static void Consume(const FunctionCallbackInfo<Value>& args) {
     Parser* parser;
     ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
-    CHECK(args[0]->IsExternal());
-    Local<External> stream_obj = args[0].As<External>();
-    StreamBase* stream = static_cast<StreamBase*>(stream_obj->Value());
+    CHECK(args[0]->IsObject());
+    StreamBase* stream = StreamBase::FromObject(args[0].As<Object>());
     CHECK_NOT_NULL(stream);
     stream->PushStreamListener(parser);
   }

diff --git 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
@@ -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<FunctionTemplate> t) {
       t, "writeUcs2String", JSMethod<&StreamBase::WriteString<UCS2>>);
   env->SetProtoMethod(
       t, "writeLatin1String", JSMethod<&StreamBase::WriteString<LATIN1>>);
+  t->PrototypeTemplate()->Set(FIXED_ONE_BYTE_STRING(env->isolate(),
+                                                    "isStreamBase"),
+                              True(env->isolate()));
 }
 
 void StreamBase::GetFD(const FunctionCallbackInfo<Value>& args) {

diff --git 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<Value>& args) {
   CHECK(args.IsConstructCall());
-  CHECK(args[0]->IsExternal());
-  CHECK(args[1]->IsExternal());
-  auto source = static_cast<StreamBase*>(args[0].As<External>()->Value());
-  auto sink = static_cast<StreamBase*>(args[1].As<External>()->Value());
+  CHECK(args[0]->IsObject());
+  CHECK(args[1]->IsObject());
+  StreamBase* source = StreamBase::FromObject(args[0].As<Object>());
+  StreamBase* sink = StreamBase::FromObject(args[1].As<Object>());
 
   new StreamPipe(source, sink, args.This());
 }

diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
@@ -151,12 +151,11 @@ void TLSWrap::Wrap(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[1]->IsObject());
   CHECK(args[2]->IsBoolean());
 
-  Local<External> stream_obj = args[0].As<External>();
   Local<Object> sc = args[1].As<Object>();
   Kind kind = args[2]->IsTrue() ? SSLWrap<TLSWrap>::kServer :
                                   SSLWrap<TLSWrap>::kClient;
 
-  StreamBase* stream = static_cast<StreamBase*>(stream_obj->Value());
+  StreamBase* stream = StreamBase::FromObject(args[0].As<Object>());
   CHECK_NOT_NULL(stream);
 
   Local<Object> obj;

diff --git 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
@@ -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);