tls_wrap: use DoTryWrite()

Use `DoTryWrite()` to write data to the underlying socket. This does probably not make any difference in performance because the callback is still deferred (for now), but brings TLSWrap in line with other things that write to streams. PR-URL: #18676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Feb 27, 2018 · efb4646 · efb4646
1 parent 7fb72a5
commit efb4646
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 31 deletions.
diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
@@ -287,17 +287,30 @@ void TLSWrap::EncOut() {
                                                                  &count);
   CHECK(write_size_ != 0 && count != 0);
 
+  uv_buf_t buf[arraysize(data)];
+  uv_buf_t* bufs = buf;
+  for (size_t i = 0; i < count; i++)
+    buf[i] = uv_buf_init(data[i], size[i]);
+
+  int err = stream_->DoTryWrite(&bufs, &count);
+  if (err != 0) {
+    InvokeQueued(err);
+  } else if (count == 0) {
+    env()->SetImmediate([](Environment* env, void* data) {
+      NODE_COUNT_NET_BYTES_SENT(write_size_);
+      static_cast<TLSWrap*>(data)->OnStreamAfterWrite(nullptr, 0);
+    }, this, object());
+    return;
+  }
+
   Local<Object> req_wrap_obj =
       env()->write_wrap_constructor_function()
           ->NewInstance(env()->context()).ToLocalChecked();
   WriteWrap* write_req = WriteWrap::New(env(),
                                         req_wrap_obj,
                                         static_cast<StreamBase*>(stream_));
 
-  uv_buf_t buf[arraysize(data)];
-  for (size_t i = 0; i < count; i++)
-    buf[i] = uv_buf_init(data[i], size[i]);
-  int err = stream_->DoWrite(write_req, buf, count, nullptr);
+  err = stream_->DoWrite(write_req, buf, count, nullptr);
 
   // Ignore errors, this should be already handled in js
   if (err) {
@@ -310,9 +323,8 @@ void TLSWrap::EncOut() {
 
 
 void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) {
-  // We should not be getting here after `DestroySSL`, because all queued writes
-  // must be invoked with UV_ECANCELED
-  CHECK_NE(ssl_, nullptr);
+  if (ssl_ == nullptr)
+    status = UV_ECANCELED;
 
   // Handle error
   if (status) {

diff --git a/test/async-hooks/test-writewrap.js b/test/async-hooks/test-writewrap.js
@@ -1,28 +1,20 @@
 'use strict';
 
 const common = require('../common');
-if (!common.hasCrypto)
-  common.skip('missing crypto');
-
 const assert = require('assert');
 const initHooks = require('./init-hooks');
-const fixtures = require('../common/fixtures');
 const { checkInvocations } = require('./hook-checks');
-const tls = require('tls');
+const net = require('net');
 
 const hooks = initHooks();
 hooks.enable();
 
 //
 // Creating server and listening on port
 //
-const server = tls
-  .createServer({
-    cert: fixtures.readSync('test_cert.pem'),
-    key: fixtures.readSync('test_key.pem')
-  })
+const server = net.createServer()
   .on('listening', common.mustCall(onlistening))
-  .on('secureConnection', common.mustCall(onsecureConnection))
+  .on('connection', common.mustCall(onconnection))
   .listen(0);
 
 assert.strictEqual(hooks.activitiesOfTypes('WRITEWRAP').length, 0);
@@ -32,16 +24,17 @@ function onlistening() {
   //
   // Creating client and connecting it to server
   //
-  tls
-    .connect(server.address().port, { rejectUnauthorized: false })
-    .on('secureConnect', common.mustCall(onsecureConnect));
+  net
+    .connect(server.address().port)
+    .on('connect', common.mustCall(onconnect));
 
   assert.strictEqual(hooks.activitiesOfTypes('WRITEWRAP').length, 0);
 }
 
 function checkDestroyedWriteWraps(n, stage) {
   const as = hooks.activitiesOfTypes('WRITEWRAP');
-  assert.strictEqual(as.length, n, `${n} WRITEWRAPs when ${stage}`);
+  assert.strictEqual(as.length, n,
+                     `${as.length} out of ${n} WRITEWRAPs when ${stage}`);
 
   function checkValidWriteWrap(w) {
     assert.strictEqual(w.type, 'WRITEWRAP');
@@ -53,41 +46,47 @@ function checkDestroyedWriteWraps(n, stage) {
   as.forEach(checkValidWriteWrap);
 }
 
-function onsecureConnection() {
+function onconnection(conn) {
+  conn.resume();
   //
   // Server received client connection
   //
-  checkDestroyedWriteWraps(3, 'server got secure connection');
+  checkDestroyedWriteWraps(0, 'server got connection');
 }
 
-function onsecureConnect() {
+function onconnect() {
   //
   // Client connected to server
   //
-  checkDestroyedWriteWraps(4, 'client connected');
+  checkDestroyedWriteWraps(0, 'client connected');
 
   //
   // Destroying client socket
   //
-  this.destroy();
+  this.write('f'.repeat(128000), () => onafterwrite(this));
+}
+
+function onafterwrite(self) {
+  checkDestroyedWriteWraps(1, 'client destroyed');
+  self.destroy();
 
-  checkDestroyedWriteWraps(4, 'client destroyed');
+  checkDestroyedWriteWraps(1, 'client destroyed');
 
   //
   // Closing server
   //
   server.close(common.mustCall(onserverClosed));
-  checkDestroyedWriteWraps(4, 'server closing');
+  checkDestroyedWriteWraps(1, 'server closing');
 }
 
 function onserverClosed() {
-  checkDestroyedWriteWraps(4, 'server closed');
+  checkDestroyedWriteWraps(1, 'server closed');
 }
 
 process.on('exit', onexit);
 
 function onexit() {
   hooks.disable();
   hooks.sanityCheck('WRITEWRAP');
-  checkDestroyedWriteWraps(4, 'process exits');
+  checkDestroyedWriteWraps(1, 'process exits');
 }