From 25ce45825f576ff9d027eea1f6380fb286111924 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 11 Dec 2017 17:55:17 -0500 Subject: [PATCH 001/206] net,src: refactor writeQueueSize tracking Currently, writeQueueSize is never used in C++ and barely used within JS. Instead of constantly updating the value on the JS object, create a getter that will retrieve the most up-to-date value from C++. For the vast majority of cases though, create a new prop on Socket.prototype[kLastWriteQueueSize] using a Symbol. Use this to track the current write size, entirely in JS land. Backport-PR-URL: https://github.com/nodejs/node/pull/18084 PR-URL: https://github.com/nodejs/node/pull/17650 Reviewed-By: Anna Henningsen --- lib/_tls_wrap.js | 5 -- lib/net.js | 33 +++++---- src/pipe_wrap.cc | 1 - src/stream_base.cc | 13 +++- src/stream_base.h | 1 + src/stream_wrap.cc | 48 +++++++------ src/stream_wrap.h | 6 +- src/tcp_wrap.cc | 1 - src/tls_wrap.cc | 45 ++++++------ src/tls_wrap.h | 5 +- src/tty_wrap.cc | 6 +- test/parallel/test-tls-buffersize.js | 10 +-- .../test-http-keep-alive-large-write.js | 67 +++++------------- .../test-https-keep-alive-large-write.js | 70 ++++--------------- 14 files changed, 126 insertions(+), 185 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index d888d777353a76..e17ea3f5948e82 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -460,11 +460,6 @@ TLSSocket.prototype._init = function(socket, wrap) { var options = this._tlsOptions; var ssl = this._handle; - // lib/net.js expect this value to be non-zero if write hasn't been flushed - // immediately. After the handshake is done this will represent the actual - // write queue size - ssl.writeQueueSize = 1; - this.server = options.server; // For clients, we will always have either a given ca list or be using diff --git a/lib/net.js b/lib/net.js index 886365092c05a6..540f7ce800d520 100644 --- a/lib/net.js +++ b/lib/net.js @@ -48,6 +48,8 @@ const { nextTick } = require('internal/process/next_tick'); const errors = require('internal/errors'); const dns = require('dns'); +const kLastWriteQueueSize = Symbol('lastWriteQueueSize'); + // `cluster` is only used by `listenInCluster` so for startup performance // reasons it's lazy loaded. var cluster = null; @@ -198,6 +200,7 @@ function Socket(options) { this._handle = null; this._parent = null; this._host = null; + this[kLastWriteQueueSize] = 0; if (typeof options === 'number') options = { fd: options }; // Legacy interface. @@ -401,12 +404,14 @@ Socket.prototype.setTimeout = function(msecs, callback) { Socket.prototype._onTimeout = function() { - if (this._handle) { - // `.prevWriteQueueSize` !== `.updateWriteQueueSize()` means there is + const handle = this._handle; + const lastWriteQueueSize = this[kLastWriteQueueSize]; + if (lastWriteQueueSize > 0 && handle) { + // `lastWriteQueueSize !== writeQueueSize` means there is // an active write in progress, so we suppress the timeout. - const prevWriteQueueSize = this._handle.writeQueueSize; - if (prevWriteQueueSize > 0 && - prevWriteQueueSize !== this._handle.updateWriteQueueSize()) { + const writeQueueSize = handle.writeQueueSize; + if (lastWriteQueueSize !== writeQueueSize) { + this[kLastWriteQueueSize] = writeQueueSize; this._unrefTimer(); return; } @@ -476,7 +481,7 @@ Object.defineProperty(Socket.prototype, 'readyState', { Object.defineProperty(Socket.prototype, 'bufferSize', { get: function() { if (this._handle) { - return this._handle.writeQueueSize + this.writableLength; + return this[kLastWriteQueueSize] + this.writableLength; } } }); @@ -767,12 +772,13 @@ Socket.prototype._writeGeneric = function(writev, data, encoding, cb) { this._bytesDispatched += req.bytes; - // If it was entirely flushed, we can write some more right now. - // However, if more is left in the queue, then wait until that clears. - if (req.async && this._handle.writeQueueSize !== 0) - req.cb = cb; - else + if (!req.async) { cb(); + return; + } + + req.cb = cb; + this[kLastWriteQueueSize] = req.bytes; }; @@ -856,6 +862,9 @@ function afterWrite(status, handle, req, err) { if (self !== process.stderr && self !== process.stdout) debug('afterWrite', status); + if (req.async) + self[kLastWriteQueueSize] = 0; + // callback may come after call to destroy. if (self.destroyed) { debug('afterWrite destroyed'); @@ -875,7 +884,7 @@ function afterWrite(status, handle, req, err) { debug('afterWrite call cb'); if (req.cb) - req.cb.call(self); + req.cb.call(undefined); } diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index 465cbf4d16dbfe..c5958a2271a83e 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -166,7 +166,6 @@ PipeWrap::PipeWrap(Environment* env, int r = uv_pipe_init(env->event_loop(), &handle_, ipc); CHECK_EQ(r, 0); // How do we proxy this error up to javascript? // Suggestion: uv_pipe_init() returns void. - UpdateWriteQueueSize(); } diff --git a/src/stream_base.cc b/src/stream_base.cc index ecb5f3dd1b954e..b1aea79d52f762 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -195,7 +195,8 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { } err = DoWrite(req_wrap, buf_list, count, nullptr); - req_wrap_obj->Set(env->async(), True(env->isolate())); + if (HasWriteQueue()) + req_wrap_obj->Set(env->async(), True(env->isolate())); if (err) req_wrap->Dispose(); @@ -253,7 +254,8 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo& args) { } err = DoWrite(req_wrap, bufs, count, nullptr); - req_wrap_obj->Set(env->async(), True(env->isolate())); + if (HasWriteQueue()) + req_wrap_obj->Set(env->async(), True(env->isolate())); req_wrap_obj->Set(env->buffer_string(), args[1]); if (err) @@ -379,7 +381,8 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { reinterpret_cast(send_handle)); } - req_wrap_obj->Set(env->async(), True(env->isolate())); + if (HasWriteQueue()) + req_wrap_obj->Set(env->async(), True(env->isolate())); if (err) req_wrap->Dispose(); @@ -473,6 +476,10 @@ int StreamResource::DoTryWrite(uv_buf_t** bufs, size_t* count) { return 0; } +bool StreamResource::HasWriteQueue() { + return true; +} + const char* StreamResource::Error() const { return nullptr; diff --git a/src/stream_base.h b/src/stream_base.h index d063176b04a4db..071627f3bf2a67 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -162,6 +162,7 @@ class StreamResource { uv_buf_t* bufs, size_t count, uv_stream_t* send_handle) = 0; + virtual bool HasWriteQueue(); virtual const char* Error() const; virtual void ClearError(); diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index f6cfba84c28a55..094991107ba7aa 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -40,13 +40,15 @@ namespace node { using v8::Context; +using v8::DontDelete; using v8::EscapableHandleScope; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; -using v8::Integer; using v8::Local; using v8::Object; +using v8::ReadOnly; +using v8::Signature; using v8::Value; @@ -99,7 +101,16 @@ LibuvStreamWrap::LibuvStreamWrap(Environment* env, void LibuvStreamWrap::AddMethods(Environment* env, v8::Local target, int flags) { - env->SetProtoMethod(target, "updateWriteQueueSize", UpdateWriteQueueSize); + Local get_write_queue_size = + FunctionTemplate::New(env->isolate(), + GetWriteQueueSize, + env->as_external(), + Signature::New(env->isolate(), target)); + target->PrototypeTemplate()->SetAccessorProperty( + env->write_queue_size_string(), + get_write_queue_size, + Local(), + static_cast(ReadOnly | DontDelete)); env->SetProtoMethod(target, "setBlocking", SetBlocking); StreamBase::AddMethods(env, target, flags); } @@ -135,17 +146,6 @@ bool LibuvStreamWrap::IsIPCPipe() { } -uint32_t LibuvStreamWrap::UpdateWriteQueueSize() { - HandleScope scope(env()->isolate()); - uint32_t write_queue_size = stream()->write_queue_size; - object()->Set(env()->context(), - env()->write_queue_size_string(), - Integer::NewFromUnsigned(env()->isolate(), - write_queue_size)).FromJust(); - return write_queue_size; -} - - int LibuvStreamWrap::ReadStart() { return uv_read_start(stream(), OnAlloc, OnRead); } @@ -267,13 +267,18 @@ void LibuvStreamWrap::OnRead(uv_stream_t* handle, } -void LibuvStreamWrap::UpdateWriteQueueSize( - const FunctionCallbackInfo& args) { +void LibuvStreamWrap::GetWriteQueueSize( + const FunctionCallbackInfo& info) { LibuvStreamWrap* wrap; - ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + ASSIGN_OR_RETURN_UNWRAP(&wrap, info.This()); + + if (wrap->stream() == nullptr) { + info.GetReturnValue().Set(0); + return; + } - uint32_t write_queue_size = wrap->UpdateWriteQueueSize(); - args.GetReturnValue().Set(write_queue_size); + uint32_t write_queue_size = wrap->stream()->write_queue_size; + info.GetReturnValue().Set(write_queue_size); } @@ -370,12 +375,16 @@ int LibuvStreamWrap::DoWrite(WriteWrap* w, } w->Dispatched(); - UpdateWriteQueueSize(); return r; } +bool LibuvStreamWrap::HasWriteQueue() { + return stream()->write_queue_size > 0; +} + + void LibuvStreamWrap::AfterUvWrite(uv_write_t* req, int status) { WriteWrap* req_wrap = WriteWrap::from_req(req); CHECK_NE(req_wrap, nullptr); @@ -387,7 +396,6 @@ void LibuvStreamWrap::AfterUvWrite(uv_write_t* req, int status) { void LibuvStreamWrap::AfterWrite(WriteWrap* w, int status) { StreamBase::AfterWrite(w, status); - UpdateWriteQueueSize(); } } // namespace node diff --git a/src/stream_wrap.h b/src/stream_wrap.h index 414bad393fa9b1..a695f9a08a7729 100644 --- a/src/stream_wrap.h +++ b/src/stream_wrap.h @@ -55,6 +55,7 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { uv_buf_t* bufs, size_t count, uv_stream_t* send_handle) override; + bool HasWriteQueue() override; inline uv_stream_t* stream() const { return stream_; @@ -83,15 +84,14 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { } AsyncWrap* GetAsyncWrap() override; - uint32_t UpdateWriteQueueSize(); static void AddMethods(Environment* env, v8::Local target, int flags = StreamBase::kFlagNone); private: - static void UpdateWriteQueueSize( - const v8::FunctionCallbackInfo& args); + static void GetWriteQueueSize( + const v8::FunctionCallbackInfo& info); static void SetBlocking(const v8::FunctionCallbackInfo& args); // Callbacks for libuv diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index bdae0ee994360c..3a0a3f295e2c72 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -170,7 +170,6 @@ TCPWrap::TCPWrap(Environment* env, Local object, ProviderType provider) int r = uv_tcp_init(env->event_loop(), &handle_); CHECK_EQ(r, 0); // How do we proxy this error up to javascript? // Suggestion: uv_tcp_init() returns void. - UpdateWriteQueueSize(); } diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 85858577c54a68..6752d16b20659b 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -35,14 +35,16 @@ namespace node { using crypto::SecureContext; using crypto::SSLWrap; using v8::Context; +using v8::DontDelete; using v8::EscapableHandleScope; using v8::Exception; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; -using v8::Integer; using v8::Local; using v8::Object; +using v8::ReadOnly; +using v8::Signature; using v8::String; using v8::Value; @@ -307,7 +309,6 @@ void TLSWrap::EncOut() { // No data to write if (BIO_pending(enc_out_) == 0) { - UpdateWriteQueueSize(); if (clear_in_->Length() == 0) InvokeQueued(0); return; @@ -553,17 +554,6 @@ bool TLSWrap::IsClosing() { } -uint32_t TLSWrap::UpdateWriteQueueSize(uint32_t write_queue_size) { - HandleScope scope(env()->isolate()); - if (write_queue_size == 0) - write_queue_size = BIO_pending(enc_out_); - object()->Set(env()->context(), - env()->write_queue_size_string(), - Integer::NewFromUnsigned(env()->isolate(), - write_queue_size)).FromJust(); - return write_queue_size; -} - int TLSWrap::ReadStart() { if (stream_ != nullptr) @@ -610,9 +600,6 @@ int TLSWrap::DoWrite(WriteWrap* w, // However, if there is any data that should be written to the socket, // the callback should not be invoked immediately if (BIO_pending(enc_out_) == 0) { - // net.js expects writeQueueSize to be > 0 if the write isn't - // immediately flushed - UpdateWriteQueueSize(1); return stream_->DoWrite(w, bufs, count, send_handle); } } @@ -665,7 +652,6 @@ int TLSWrap::DoWrite(WriteWrap* w, // Try writing data immediately EncOut(); - UpdateWriteQueueSize(); return 0; } @@ -937,12 +923,17 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) { #endif // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB -void TLSWrap::UpdateWriteQueueSize(const FunctionCallbackInfo& args) { +void TLSWrap::GetWriteQueueSize(const FunctionCallbackInfo& info) { TLSWrap* wrap; - ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + ASSIGN_OR_RETURN_UNWRAP(&wrap, info.This()); - uint32_t write_queue_size = wrap->UpdateWriteQueueSize(); - args.GetReturnValue().Set(write_queue_size); + if (wrap->clear_in_ == nullptr) { + info.GetReturnValue().Set(0); + return; + } + + uint32_t write_queue_size = BIO_pending(wrap->enc_out_); + info.GetReturnValue().Set(write_queue_size); } @@ -965,6 +956,17 @@ void TLSWrap::Initialize(Local target, t->InstanceTemplate()->SetInternalFieldCount(1); t->SetClassName(tlsWrapString); + Local get_write_queue_size = + FunctionTemplate::New(env->isolate(), + GetWriteQueueSize, + env->as_external(), + Signature::New(env->isolate(), t)); + t->PrototypeTemplate()->SetAccessorProperty( + env->write_queue_size_string(), + get_write_queue_size, + Local(), + static_cast(ReadOnly | DontDelete)); + AsyncWrap::AddWrapMethods(env, t, AsyncWrap::kFlagHasReset); env->SetProtoMethod(t, "receive", Receive); env->SetProtoMethod(t, "start", Start); @@ -972,7 +974,6 @@ void TLSWrap::Initialize(Local target, env->SetProtoMethod(t, "enableSessionCallbacks", EnableSessionCallbacks); env->SetProtoMethod(t, "destroySSL", DestroySSL); env->SetProtoMethod(t, "enableCertCb", EnableCertCb); - env->SetProtoMethod(t, "updateWriteQueueSize", UpdateWriteQueueSize); StreamBase::AddMethods(env, t, StreamBase::kFlagHasWritev); SSLWrap::AddMethods(env, t); diff --git a/src/tls_wrap.h b/src/tls_wrap.h index cd6a46b7d9b32d..ffa4fb5b4d5c31 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -118,7 +118,6 @@ class TLSWrap : public AsyncWrap, AsyncWrap* GetAsyncWrap() override; bool IsIPCPipe() override; - uint32_t UpdateWriteQueueSize(uint32_t write_queue_size = 0); // Resource implementation static void OnAfterWriteImpl(WriteWrap* w, int status, void* ctx); @@ -174,8 +173,8 @@ class TLSWrap : public AsyncWrap, bool eof_; private: - static void UpdateWriteQueueSize( - const v8::FunctionCallbackInfo& args); + static void GetWriteQueueSize( + const v8::FunctionCallbackInfo& info); }; } // namespace node diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc index 18f9feca57a4c2..111c568cb52a8a 100644 --- a/src/tty_wrap.cc +++ b/src/tty_wrap.cc @@ -153,11 +153,9 @@ void TTYWrap::New(const FunctionCallbackInfo& args) { CHECK_GE(fd, 0); int err = 0; - TTYWrap* wrap = new TTYWrap(env, args.This(), fd, args[1]->IsTrue(), &err); + new TTYWrap(env, args.This(), fd, args[1]->IsTrue(), &err); if (err != 0) - return env->ThrowUVException(err, "uv_tty_init"); - - wrap->UpdateWriteQueueSize(); + env->ThrowUVException(err, "uv_tty_init"); } diff --git a/test/parallel/test-tls-buffersize.js b/test/parallel/test-tls-buffersize.js index 49848cd865aca5..c94b95d7b32d31 100644 --- a/test/parallel/test-tls-buffersize.js +++ b/test/parallel/test-tls-buffersize.js @@ -7,17 +7,17 @@ const fixtures = require('../common/fixtures'); const tls = require('tls'); const iter = 10; -const overhead = 30; const server = tls.createServer({ key: fixtures.readKey('agent2-key.pem'), cert: fixtures.readKey('agent2-cert.pem') }, common.mustCall((socket) => { - socket.on('readable', common.mustCallAtLeast(() => { - socket.read(); - }, 1)); + let str = ''; + socket.setEncoding('utf-8'); + socket.on('data', (chunk) => { str += chunk; }); socket.on('end', common.mustCall(() => { + assert.strictEqual(str, 'a'.repeat(iter - 1)); server.close(); })); })); @@ -31,7 +31,7 @@ server.listen(0, common.mustCall(() => { for (let i = 1; i < iter; i++) { client.write('a'); - assert.strictEqual(client.bufferSize, i + overhead); + assert.strictEqual(client.bufferSize, i + 1); } client.on('finish', common.mustCall(() => { diff --git a/test/sequential/test-http-keep-alive-large-write.js b/test/sequential/test-http-keep-alive-large-write.js index 2cdf539e76b2fc..4119c2353daa53 100644 --- a/test/sequential/test-http-keep-alive-large-write.js +++ b/test/sequential/test-http-keep-alive-large-write.js @@ -6,26 +6,12 @@ const http = require('http'); // This test assesses whether long-running writes can complete // or timeout because the socket is not aware that the backing // stream is still writing. -// To simulate a slow client, we write a really large chunk and -// then proceed through the following cycle: -// 1) Receive first 'data' event and record currently written size -// 2) Once we've read up to currently written size recorded above, -// we pause the stream and wait longer than the server timeout -// 3) Socket.prototype._onTimeout triggers and should confirm -// that the backing stream is still active and writing -// 4) Our timer fires, we resume the socket and start at 1) -const minReadSize = 250000; -const serverTimeout = common.platformTimeout(500); -let offsetTimeout = common.platformTimeout(100); -let serverConnectionHandle; -let writeSize = 3000000; -let didReceiveData = false; -// this represents each cycles write size, where the cycle consists -// of `write > read > _onTimeout` -let currentWriteSize = 0; +const writeSize = 3000000; +let socket; const server = http.createServer(common.mustCall((req, res) => { + server.close(); const content = Buffer.alloc(writeSize, 0x44); res.writeHead(200, { @@ -34,47 +20,28 @@ const server = http.createServer(common.mustCall((req, res) => { 'Vary': 'Accept-Encoding' }); - serverConnectionHandle = res.socket._handle; + socket = res.socket; + const onTimeout = socket._onTimeout; + socket._onTimeout = common.mustCallAtLeast(() => onTimeout.call(socket), 1); res.write(content); res.end(); })); -server.setTimeout(serverTimeout); server.on('timeout', () => { - assert.strictEqual(didReceiveData, false, 'Should not timeout'); + // TODO(apapirovski): This test is faulty on certain Windows systems + // as no queue is ever created + assert(!socket._handle || socket._handle.writeQueueSize === 0, + 'Should not timeout'); }); server.listen(0, common.mustCall(() => { http.get({ path: '/', port: server.address().port - }, common.mustCall((res) => { - const resume = () => res.resume(); - let receivedBufferLength = 0; - let firstReceivedAt; - res.on('data', common.mustCallAtLeast((buf) => { - if (receivedBufferLength === 0) { - currentWriteSize = Math.max( - minReadSize, - writeSize - serverConnectionHandle.writeQueueSize - ); - didReceiveData = false; - firstReceivedAt = Date.now(); - } - receivedBufferLength += buf.length; - if (receivedBufferLength >= currentWriteSize) { - didReceiveData = true; - writeSize = serverConnectionHandle.writeQueueSize; - receivedBufferLength = 0; - res.pause(); - setTimeout( - resume, - serverTimeout + offsetTimeout - (Date.now() - firstReceivedAt) - ); - offsetTimeout = 0; - } - }, 1)); - res.on('end', common.mustCall(() => { - server.close(); - })); - })); + }, (res) => { + res.once('data', () => { + socket._onTimeout(); + res.on('data', () => {}); + }); + res.on('end', () => server.close()); + }); })); diff --git a/test/sequential/test-https-keep-alive-large-write.js b/test/sequential/test-https-keep-alive-large-write.js index 5048f4f9519449..79381ba8735756 100644 --- a/test/sequential/test-https-keep-alive-large-write.js +++ b/test/sequential/test-https-keep-alive-large-write.js @@ -2,31 +2,15 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); -const assert = require('assert'); const fixtures = require('../common/fixtures'); const https = require('https'); // This test assesses whether long-running writes can complete // or timeout because the socket is not aware that the backing // stream is still writing. -// To simulate a slow client, we write a really large chunk and -// then proceed through the following cycle: -// 1) Receive first 'data' event and record currently written size -// 2) Once we've read up to currently written size recorded above, -// we pause the stream and wait longer than the server timeout -// 3) Socket.prototype._onTimeout triggers and should confirm -// that the backing stream is still active and writing -// 4) Our timer fires, we resume the socket and start at 1) -const minReadSize = 250000; -const serverTimeout = common.platformTimeout(500); -let offsetTimeout = common.platformTimeout(100); -let serverConnectionHandle; -let writeSize = 2000000; -let didReceiveData = false; -// this represents each cycles write size, where the cycle consists -// of `write > read > _onTimeout` -let currentWriteSize = 0; +const writeSize = 30000000; +let socket; const server = https.createServer({ key: fixtures.readKey('agent1-key.pem'), @@ -40,50 +24,24 @@ const server = https.createServer({ 'Vary': 'Accept-Encoding' }); - serverConnectionHandle = res.socket._handle; - res.write(content, () => { - assert.strictEqual(serverConnectionHandle.writeQueueSize, 0); - }); + socket = res.socket; + const onTimeout = socket._onTimeout; + socket._onTimeout = common.mustCallAtLeast(() => onTimeout.call(socket), 1); + res.write(content); res.end(); })); -server.setTimeout(serverTimeout); -server.on('timeout', () => { - assert.strictEqual(didReceiveData, false, 'Should not timeout'); -}); +server.on('timeout', common.mustNotCall()); server.listen(0, common.mustCall(() => { https.get({ path: '/', port: server.address().port, rejectUnauthorized: false - }, common.mustCall((res) => { - const resume = () => res.resume(); - let receivedBufferLength = 0; - let firstReceivedAt; - res.on('data', common.mustCallAtLeast((buf) => { - if (receivedBufferLength === 0) { - currentWriteSize = Math.max( - minReadSize, - writeSize - serverConnectionHandle.writeQueueSize - ); - didReceiveData = false; - firstReceivedAt = Date.now(); - } - receivedBufferLength += buf.length; - if (receivedBufferLength >= currentWriteSize) { - didReceiveData = true; - writeSize = serverConnectionHandle.writeQueueSize; - receivedBufferLength = 0; - res.pause(); - setTimeout( - resume, - serverTimeout + offsetTimeout - (Date.now() - firstReceivedAt) - ); - offsetTimeout = 0; - } - }, 1)); - res.on('end', common.mustCall(() => { - server.close(); - })); - })); + }, (res) => { + res.once('data', () => { + socket._onTimeout(); + res.on('data', () => {}); + }); + res.on('end', () => server.close()); + }); })); From 3725d4ccea4fa0a66f73508831934f64a2bd498c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 27 Dec 2017 19:27:29 +0100 Subject: [PATCH 002/206] tls: remove cleartext input data queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TLS implementation previously kept a separate buffer for incoming pieces of data, into which buffers were copied before they were up for writing. This removes this buffer, and replaces it with a simple list of `uv_buf_t`s: - The previous implementation copied all incoming data into that buffer, both allocating new storage and wasting time with copy operations. Node’s streams/net implementation already has to make sure that the allocated memory stays fresh until the write is finished, since that is what libuv streams rely on anyway. - The fact that a separate kind of buffer, `crypto::NodeBIO` was used, was confusing: These `BIO` instances are only used to communicate with openssl’s streams system otherwise, whereas this one was purely for internal memory management. - The name `clear_in_` was not very helpful. PR-URL: https://github.com/nodejs/node/pull/17883 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Tobias Nießen --- src/tls_wrap.cc | 62 +++++++++++++++++++------------------------------ src/tls_wrap.h | 3 +-- src/util-inl.h | 13 ----------- src/util.h | 1 - 4 files changed, 25 insertions(+), 54 deletions(-) diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 6752d16b20659b..78cc20810532c7 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -62,7 +62,6 @@ TLSWrap::TLSWrap(Environment* env, stream_(stream), enc_in_(nullptr), enc_out_(nullptr), - clear_in_(nullptr), write_size_(0), started_(false), established_(false), @@ -95,8 +94,6 @@ TLSWrap::TLSWrap(Environment* env, TLSWrap::~TLSWrap() { enc_in_ = nullptr; enc_out_ = nullptr; - delete clear_in_; - clear_in_ = nullptr; sc_ = nullptr; @@ -119,11 +116,6 @@ TLSWrap::~TLSWrap() { } -void TLSWrap::MakePending() { - write_callback_scheduled_ = true; -} - - bool TLSWrap::InvokeQueued(int status, const char* error_str) { if (!write_callback_scheduled_) return false; @@ -183,10 +175,6 @@ void TLSWrap::InitSSL() { // Unexpected ABORT(); } - - // Initialize ring for queud clear data - clear_in_ = new crypto::NodeBIO(); - clear_in_->AssignEnvironment(env()); } @@ -302,14 +290,14 @@ void TLSWrap::EncOut() { // Split-off queue if (established_ && current_write_ != nullptr) - MakePending(); + write_callback_scheduled_ = true; if (ssl_ == nullptr) return; // No data to write if (BIO_pending(enc_out_) == 0) { - if (clear_in_->Length() == 0) + if (pending_cleartext_input_.empty()) InvokeQueued(0); return; } @@ -496,21 +484,24 @@ bool TLSWrap::ClearIn() { if (ssl_ == nullptr) return false; + std::vector buffers; + buffers.swap(pending_cleartext_input_); + crypto::MarkPopErrorOnReturn mark_pop_error_on_return; + size_t i; int written = 0; - while (clear_in_->Length() > 0) { - size_t avail = 0; - char* data = clear_in_->Peek(&avail); + for (i = 0; i < buffers.size(); ++i) { + size_t avail = buffers[i].len; + char* data = buffers[i].base; written = SSL_write(ssl_, data, avail); CHECK(written == -1 || written == static_cast(avail)); if (written == -1) break; - clear_in_->Read(nullptr, avail); } // All written - if (clear_in_->Length() == 0) { + if (i == buffers.size()) { CHECK_GE(written, 0); return true; } @@ -520,9 +511,15 @@ bool TLSWrap::ClearIn() { std::string error_str; Local arg = GetSSLError(written, &err, &error_str); if (!arg.IsEmpty()) { - MakePending(); + write_callback_scheduled_ = true; InvokeQueued(UV_EPROTO, error_str.c_str()); - clear_in_->Reset(); + } else { + // Push back the not-yet-written pending buffers into their queue. + // This can be skipped in the error case because no further writes + // would succeed anyway. + pending_cleartext_input_.insert(pending_cleartext_input_.end(), + &buffers[i], + &buffers[buffers.size()]); } return false; @@ -615,14 +612,6 @@ int TLSWrap::DoWrite(WriteWrap* w, return 0; } - // Process enqueued data first - if (!ClearIn()) { - // If there're still data to process - enqueue current one - for (i = 0; i < count; i++) - clear_in_->Write(bufs[i].base, bufs[i].len); - return 0; - } - if (ssl_ == nullptr) { ClearError(); error_ = "Write after DestroySSL"; @@ -645,9 +634,9 @@ int TLSWrap::DoWrite(WriteWrap* w, if (!arg.IsEmpty()) return UV_EPROTO; - // No errors, queue rest - for (; i < count; i++) - clear_in_->Write(bufs[i].base, bufs[i].len); + pending_cleartext_input_.insert(pending_cleartext_input_.end(), + &bufs[i], + &bufs[count]); } // Try writing data immediately @@ -817,17 +806,14 @@ void TLSWrap::DestroySSL(const FunctionCallbackInfo& args) { TLSWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - // Move all writes to pending - wrap->MakePending(); + // If there is a write happening, mark it as finished. + wrap->write_callback_scheduled_ = true; // And destroy wrap->InvokeQueued(UV_ECANCELED, "Canceled because of SSL destruction"); // Destroy the SSL structure and friends wrap->SSLWrap::DestroySSL(); - - delete wrap->clear_in_; - wrap->clear_in_ = nullptr; } @@ -927,7 +913,7 @@ void TLSWrap::GetWriteQueueSize(const FunctionCallbackInfo& info) { TLSWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, info.This()); - if (wrap->clear_in_ == nullptr) { + if (wrap->ssl_ == nullptr) { info.GetReturnValue().Set(0); return; } diff --git a/src/tls_wrap.h b/src/tls_wrap.h index ffa4fb5b4d5c31..ae83c82c3226fd 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -101,7 +101,6 @@ class TLSWrap : public AsyncWrap, void EncOutAfterWrite(WriteWrap* req_wrap, int status); bool ClearIn(); void ClearOut(); - void MakePending(); bool InvokeQueued(int status, const char* error_str = nullptr); inline void Cycle() { @@ -158,7 +157,7 @@ class TLSWrap : public AsyncWrap, StreamBase* stream_; BIO* enc_in_; BIO* enc_out_; - crypto::NodeBIO* clear_in_; + std::vector pending_cleartext_input_; size_t write_size_; WriteWrap* current_write_ = nullptr; bool write_callback_scheduled_ = false; diff --git a/src/util-inl.h b/src/util-inl.h index c3855eba098c67..558a0ab2b42611 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -99,19 +99,6 @@ ListHead::~ListHead() { head_.next_->Remove(); } -template (T::*M)> -void ListHead::MoveBack(ListHead* that) { - if (IsEmpty()) - return; - ListNode* to = &that->head_; - head_.next_->prev_ = to->prev_; - to->prev_->next_ = head_.next_; - head_.prev_->next_ = to; - to->prev_ = head_.prev_; - head_.prev_ = &head_; - head_.next_ = &head_; -} - template (T::*M)> void ListHead::PushBack(T* element) { ListNode* that = &(element->*M); diff --git a/src/util.h b/src/util.h index eb060a57b8801b..47bdf27c307109 100644 --- a/src/util.h +++ b/src/util.h @@ -181,7 +181,6 @@ class ListHead { inline ListHead() = default; inline ~ListHead(); - inline void MoveBack(ListHead* that); inline void PushBack(T* element); inline void PushFront(T* element); inline bool IsEmpty() const; From 11566fe5325cc0ddb417e6b64e4eec3aebae3b90 Mon Sep 17 00:00:00 2001 From: Jan Krems Date: Thu, 9 Nov 2017 06:46:20 -0800 Subject: [PATCH 003/206] deps: cherry-pick dbfe4a49d8 from upstream V8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: Introduce ScriptOrModule and HostDefinedOptions This patch introduces a new container type ScriptOrModule which provides the name and the host defined options of the script/module. This patch also introduces a new PrimitivesArray that can hold Primitive values, which the embedder can use to store metadata. The HostDefinedOptions is passed to V8 through the ScriptOrigin, and passed back to the embedder through HostImportModuleDynamically for module loading. Bug: v8:5785, v8:6658, v8:6683 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Change-Id: I56c26fc9a680b273ac0a6691e5ad75f15b8dc80a Reviewed-on: https://chromium-review.googlesource.com/622158 Reviewed-by: Adam Klein Reviewed-by: Georg Neis Commit-Queue: Sathya Gunasekaran Cr-Commit-Position: refs/heads/master@{#47724} Backport-PR-URL: https://github.com/nodejs/node/pull/17823 PR-URL: https://github.com/nodejs/node/pull/16889 Refs: https://github.com/v8/v8/commit/dbfe4a49d88f6655ed31285a40b63786ab1e8e49 Refs: https://github.com/nodejs/node/pull/15713 Reviewed-By: Ben Noordhuis Reviewed-By: Michaël Zasso --- common.gypi | 2 +- deps/v8/include/v8.h | 75 ++++++++++++++++-- deps/v8/src/api.cc | 81 ++++++++++++++++++- deps/v8/src/api.h | 14 +++- deps/v8/src/bootstrapper.cc | 4 + deps/v8/src/compiler.cc | 7 ++ deps/v8/src/compiler.h | 2 + deps/v8/src/d8.cc | 8 +- deps/v8/src/d8.h | 3 +- deps/v8/src/factory.cc | 3 +- deps/v8/src/isolate.cc | 4 +- deps/v8/src/isolate.h | 2 +- deps/v8/src/objects/script-inl.h | 2 + deps/v8/src/objects/script.h | 8 ++ deps/v8/src/runtime/runtime-module.cc | 5 +- deps/v8/test/cctest/compiler/test-linkage.cc | 3 + deps/v8/test/cctest/test-api.cc | 82 +++++++++++++++++++- deps/v8/test/cctest/test-compiler.cc | 3 + deps/v8/test/cctest/test-serialize.cc | 5 ++ deps/v8/tools/v8heapconst.py | 36 ++++----- 20 files changed, 302 insertions(+), 47 deletions(-) diff --git a/common.gypi b/common.gypi index 0ee48812415799..0108301e3fb28c 100644 --- a/common.gypi +++ b/common.gypi @@ -27,7 +27,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.18', + 'v8_embedder_string': '-node.19', # Enable disassembler for `--print-code` v8 options 'v8_enable_disassembler': 1, diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h index b183d4342fcbcf..7393dc6cbcc13a 100644 --- a/deps/v8/include/v8.h +++ b/deps/v8/include/v8.h @@ -104,6 +104,7 @@ class String; class StringObject; class Symbol; class SymbolObject; +class PrimitiveArray; class Private; class Uint32; class Utils; @@ -978,6 +979,48 @@ class V8_EXPORT Data { Data(); }; +/** + * This is an unfinished experimental feature, and is only exposed + * here for internal testing purposes. DO NOT USE. + * + * A container type that holds relevant metadata for module loading. + * + * This is passed back to the embedder as part of + * HostImportDynamicallyCallback for module loading. + */ +class V8_EXPORT ScriptOrModule { + public: + /** + * The name that was passed by the embedder as ResourceName to the + * ScriptOrigin. This can be either a v8::String or v8::Undefined. + */ + Local GetResourceName(); + + /** + * The options that were passed by the embedder as HostDefinedOptions to + * the ScriptOrigin. + */ + Local GetHostDefinedOptions(); +}; + +/** + * This is an unfinished experimental feature, and is only exposed + * here for internal testing purposes. DO NOT USE. + * + * An array to hold Primitive values. This is used by the embedder to + * pass host defined options to the ScriptOptions during compilation. + * + * This is passed back to the embedder as part of + * HostImportDynamicallyCallback for module loading. + * + */ +class V8_EXPORT PrimitiveArray { + public: + static Local New(Isolate* isolate, int length); + int Length() const; + void Set(int index, Local item); + Local Get(int index); +}; /** * The optional attributes of ScriptOrigin. @@ -1027,13 +1070,17 @@ class ScriptOrigin { Local source_map_url = Local(), Local resource_is_opaque = Local(), Local is_wasm = Local(), - Local is_module = Local()); + Local is_module = Local() /*, + // Backed out for ABI compatibility with V8 6.2 + Local host_defined_options = Local() */); V8_INLINE Local ResourceName() const; V8_INLINE Local ResourceLineOffset() const; V8_INLINE Local ResourceColumnOffset() const; V8_INLINE Local ScriptID() const; V8_INLINE Local SourceMapUrl() const; + // Backed out for ABI compatibility with V8 6.2 + // V8_INLINE Local HostDefinedOptions() const; V8_INLINE ScriptOriginOptions Options() const { return options_; } private: @@ -1043,6 +1090,8 @@ class ScriptOrigin { ScriptOriginOptions options_; Local script_id_; Local source_map_url_; + // Backed out for ABI compatibility with V8 6.2 + // Local host_defined_options_; }; /** @@ -1289,6 +1338,7 @@ class V8_EXPORT ScriptCompiler { Local resource_column_offset; ScriptOriginOptions resource_options; Local source_map_url; + // Local host_defined_options; // Cached data from previous compilation (if a kConsume*Cache flag is // set), or hold newly generated cache data (kProduce*Cache flags) are @@ -6209,8 +6259,8 @@ typedef void (*DeprecatedCallCompletedCallback)(); * embedder to load a module. This is used as part of the dynamic * import syntax. * - * The referrer is the name of the file which calls the dynamic - * import. The referrer can be used to resolve the module location. + * The referrer contains metadata about the script/module that calls + * import. * * The specifier is the name of the module that should be imported. * @@ -6225,7 +6275,8 @@ typedef void (*DeprecatedCallCompletedCallback)(); * that exception by returning an empty MaybeLocal. */ typedef MaybeLocal (*HostImportModuleDynamicallyCallback)( - Local context, Local referrer, Local specifier); + Local context, Local referrer, + Local specifier); /** * PromiseHook with type kInit is called when a new promise is @@ -9545,7 +9596,9 @@ ScriptOrigin::ScriptOrigin(Local resource_name, Local script_id, Local source_map_url, Local resource_is_opaque, - Local is_wasm, Local is_module) + Local is_wasm, Local is_module /*, + // Backed out for ABI compatibility with V8 6.2 + Local host_defined_options */) : resource_name_(resource_name), resource_line_offset_(resource_line_offset), resource_column_offset_(resource_column_offset), @@ -9555,10 +9608,16 @@ ScriptOrigin::ScriptOrigin(Local resource_name, !is_wasm.IsEmpty() && is_wasm->IsTrue(), !is_module.IsEmpty() && is_module->IsTrue()), script_id_(script_id), - source_map_url_(source_map_url) {} + source_map_url_(source_map_url) /*, + // Backed out for ABI compatibility with V8 6.2 + host_defined_options_(host_defined_options) */ {} Local ScriptOrigin::ResourceName() const { return resource_name_; } +// Backed out for ABI compatibility with V8 6.2 +// Local ScriptOrigin::HostDefinedOptions() const { +// return host_defined_options_; +// } Local ScriptOrigin::ResourceLineOffset() const { return resource_line_offset_; @@ -9575,7 +9634,6 @@ Local ScriptOrigin::ScriptID() const { return script_id_; } Local ScriptOrigin::SourceMapUrl() const { return source_map_url_; } - ScriptCompiler::Source::Source(Local string, const ScriptOrigin& origin, CachedData* data) : source_string(string), @@ -9584,9 +9642,10 @@ ScriptCompiler::Source::Source(Local string, const ScriptOrigin& origin, resource_column_offset(origin.ResourceColumnOffset()), resource_options(origin.Options()), source_map_url(origin.SourceMapUrl()), + // Backed out for ABI compatibility with V8 6.2 + // host_defined_options(origin.HostDefinedOptions()), cached_data(data) {} - ScriptCompiler::Source::Source(Local string, CachedData* data) : source_string(string), cached_data(data) {} diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc index c938db9b579429..bd4a0fde702a7b 100644 --- a/deps/v8/src/api.cc +++ b/deps/v8/src/api.cc @@ -278,6 +278,9 @@ static ScriptOrigin GetScriptOriginForScript(i::Isolate* isolate, i::Handle script) { i::Handle scriptName(script->GetNameOrSourceURL(), isolate); i::Handle source_map_url(script->source_mapping_url(), isolate); + // Backed out for ABI compatibility with V8 6.2 + // i::Handle host_defined_options(script->host_defined_options(), + // isolate); v8::Isolate* v8_isolate = reinterpret_cast(script->GetIsolate()); ScriptOriginOptions options(script->origin_options()); @@ -290,7 +293,9 @@ static ScriptOrigin GetScriptOriginForScript(i::Isolate* isolate, Utils::ToLocal(source_map_url), v8::Boolean::New(v8_isolate, options.IsOpaque()), v8::Boolean::New(v8_isolate, script->type() == i::Script::TYPE_WASM), - v8::Boolean::New(v8_isolate, options.IsModule())); + v8::Boolean::New(v8_isolate, options.IsModule()) /*, + // Backed out for ABI compatibility with V8 6.2 + Utils::ToLocal(host_defined_options) */); return origin; } @@ -2082,6 +2087,23 @@ Local Script::Run() { RETURN_TO_LOCAL_UNCHECKED(Run(context), Value); } +Local ScriptOrModule::GetResourceName() { + i::Handle obj = Utils::OpenHandle(this); + i::Isolate* isolate = obj->GetIsolate(); + ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); + i::Handle val(obj->name(), isolate); + return ToApiHandle(val); +} + +Local ScriptOrModule::GetHostDefinedOptions() { + i::Handle obj = Utils::OpenHandle(this); + i::Isolate* isolate = obj->GetIsolate(); + ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); + // Backed out for ABI compatibility with V8 6.2 + // i::Handle val(obj->host_defined_options(), isolate); + // return ToApiHandle(val); + return Local(); +} Local Script::GetUnboundScript() { i::Handle obj = Utils::OpenHandle(this); @@ -2089,6 +2111,46 @@ Local Script::GetUnboundScript() { i::Handle(i::JSFunction::cast(*obj)->shared())); } +// static +Local PrimitiveArray::New(Isolate* v8_isolate, int length) { + i::Isolate* isolate = reinterpret_cast(v8_isolate); + ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); + Utils::ApiCheck(length >= 0, "v8::PrimitiveArray::New", + "length must be equal or greater than zero"); + i::Handle array = isolate->factory()->NewFixedArray(length); + return ToApiHandle(array); +} + +int PrimitiveArray::Length() const { + i::Handle array = Utils::OpenHandle(this); + i::Isolate* isolate = array->GetIsolate(); + ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); + return array->length(); +} + +void PrimitiveArray::Set(int index, Local item) { + i::Handle array = Utils::OpenHandle(this); + i::Isolate* isolate = array->GetIsolate(); + ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); + Utils::ApiCheck(index >= 0 && index < array->length(), + "v8::PrimitiveArray::Set", + "index must be greater than or equal to 0 and less than the " + "array length"); + i::Handle i_item = Utils::OpenHandle(*item); + array->set(index, *i_item); +} + +Local PrimitiveArray::Get(int index) { + i::Handle array = Utils::OpenHandle(this); + i::Isolate* isolate = array->GetIsolate(); + ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); + Utils::ApiCheck(index >= 0 && index < array->length(), + "v8::PrimitiveArray::Get", + "index must be greater than or equal to 0 and less than the " + "array length"); + i::Handle i_item(array->get(index), isolate); + return ToApiHandle(i_item); +} Module::Status Module::GetStatus() const { i::Handle self = Utils::OpenHandle(this); @@ -2225,11 +2287,18 @@ MaybeLocal ScriptCompiler::CompileUnboundInternal( TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"), "V8.CompileScript"); i::Handle name_obj; i::Handle source_map_url; + // Backed out for ABI compatibility with V8 6.2 + // i::Handle host_defined_options = + // isolate->factory()->empty_fixed_array(); int line_offset = 0; int column_offset = 0; if (!source->resource_name.IsEmpty()) { name_obj = Utils::OpenHandle(*(source->resource_name)); } + // Backed out for ABI compatibility with V8 6.2 + // if (!source->host_defined_options.IsEmpty()) { + // host_defined_options = Utils::OpenHandle(*(source->host_defined_options)); + // } if (!source->resource_line_offset.IsEmpty()) { line_offset = static_cast(source->resource_line_offset->Value()); } @@ -2243,7 +2312,7 @@ MaybeLocal ScriptCompiler::CompileUnboundInternal( result = i::Compiler::GetSharedFunctionInfoForScript( str, name_obj, line_offset, column_offset, source->resource_options, source_map_url, isolate->native_context(), NULL, &script_data, options, - i::NOT_NATIVES_CODE); + i::NOT_NATIVES_CODE /*, host_defined_options */); has_pending_exception = result.is_null(); if (has_pending_exception && script_data != NULL) { // This case won't happen during normal operation; we have compiled @@ -2508,6 +2577,10 @@ MaybeLocal