From 1483ef16a99dd4ca922990b53119587946484dcd Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 14 Jul 2019 21:16:52 +0200 Subject: [PATCH] http: clientRequest.abort is destroy --- doc/api/deprecations.md | 29 +++++ doc/api/http.md | 45 +++---- lib/_http_client.js | 118 +++++++++++------- lib/internal/streams/destroy.js | 23 ++-- ...st-http-agent-uninitialized-with-handle.js | 1 + test/parallel/test-http-client-aborted.js | 57 +++++++++ test/parallel/test-http-client-close-event.js | 4 +- test/parallel/test-http-client-set-timeout.js | 4 +- 8 files changed, 194 insertions(+), 87 deletions(-) create mode 100644 test/parallel/test-http-client-aborted.js diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 50303baa78207f..8c17d434c398f9 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2548,6 +2548,33 @@ APIs that do not make sense to use in userland. File streams should always be opened through their corresponding factory methods [`fs.createWriteStream()`][] and [`fs.createReadStream()`][]) or by passing a file descriptor in options. + +### DEP0XXX: ClientRequest.abort() is the same ClientRequest.destroy() + + +Type: Documentation-only + +[`ClientRequest.destroy()`][] is the same as [`ClientRequest.abort()`][]. + + +### DEP0XXX: ClientRequest.aborted is the same ClientRequest.destroyed + + +Type: Documentation-only + +[`ClientRequest.destroyed`][] is the same as [`ClientRequest.aborted`][]. + + [`--pending-deprecation`]: cli.html#cli_pending_deprecation [`--throw-deprecation`]: cli.html#cli_throw_deprecation [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size @@ -2555,6 +2582,8 @@ and [`fs.createReadStream()`][]) or by passing a file descriptor in options. [`Buffer.from(buffer)`]: buffer.html#buffer_class_method_buffer_from_buffer [`Buffer.isBuffer()`]: buffer.html#buffer_class_method_buffer_isbuffer_obj [`Cipher`]: crypto.html#crypto_class_cipher +[`ClientRequest.abort()`]: http.html#http_request_abort +[`ClientRequest.destroy()`]: stream.html#stream_readable_destroy_error [`Decipher`]: crypto.html#crypto_class_decipher [`EventEmitter.listenerCount(emitter, eventName)`]: events.html#events_eventemitter_listenercount_emitter_eventname [`REPLServer.clearBufferedCommand()`]: repl.html#repl_replserver_clearbufferedcommand diff --git a/doc/api/http.md b/doc/api/http.md index 44bea87e3bc6ca..3e12ae773916b3 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -565,10 +565,14 @@ srv.listen(1337, '127.0.0.1', () => { ### request.abort() +> Stability: 0 - Deprecated. Use [`request.destroy()`][] instead. + Marks the request as aborting. Calling this will cause remaining data -in the response to be dropped and the socket to be destroyed. +in the response to be dropped and the socket to be destroyed. After +calling this method, no further errors will be emitted. ### request.aborted +> Stability: 0 - Deprecated. Use [`request.destroyed`][] instead. + * {boolean} The `request.aborted` property will be `true` if the request has @@ -2319,43 +2325,24 @@ In the case of a connection error, the following events will be emitted: * `'error'` * `'close'` -In the case of a premature connection close before the response is received, -the following events will be emitted in the following order: - -* `'socket'` -* `'error'` with an error with message `'Error: socket hang up'` and code - `'ECONNRESET'` -* `'close'` - -In the case of a premature connection close after the response is received, -the following events will be emitted in the following order: - -* `'socket'` -* `'response'` - * `'data'` any number of times, on the `res` object -* (connection closed here) -* `'aborted'` on the `res` object -* `'close'` -* `'close'` on the `res` object - -If `req.abort()` is called before the connection succeeds, the following events -will be emitted in the following order: +If `req.destroy()` is called before the connection succeeds, the following +events will be emitted in the following order: * `'socket'` -* (`req.abort()` called here) +* (`req.destroy(err)` called here) * `'abort'` -* `'error'` with an error with message `'Error: socket hang up'` and code - `'ECONNRESET'` +* `'error'` if `err` was provided in `req.destroy(err)`. * `'close'` -If `req.abort()` is called after the response is received, the following events -will be emitted in the following order: +If `req.destroy()` is called after the response is received, the following +events will be emitted in the following order: * `'socket'` * `'response'` * `'data'` any number of times, on the `res` object -* (`req.abort()` called here) +* (`req.destroy(err)` called here) * `'abort'` +* `'error'` if `err` was provided in `req.destroy(err)`. * `'aborted'` on the `res` object * `'close'` * `'close'` on the `res` object @@ -2392,6 +2379,8 @@ not abort the request or do anything besides add a `'timeout'` event. [`net.createConnection()`]: net.html#net_net_createconnection_options_connectlistener [`new URL()`]: url.html#url_constructor_new_url_input_base [`removeHeader(name)`]: #http_request_removeheader_name +[`request.destroy()`]: stream.html#stream_readable_destroy_error +[`request.destroyed`]: stream.html#stream_readable_destroyed [`request.end()`]: #http_request_end_data_encoding_callback [`request.flushHeaders()`]: #http_request_flushheaders [`request.getHeader()`]: #http_request_getheader_name diff --git a/lib/_http_client.js b/lib/_http_client.js index 30a6366f355708..908cc84cefb8fe 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -23,6 +23,7 @@ const { Object } = primordials; +const { destroy, kWritableState } = require('internal/streams/destroy'); const net = require('net'); const url = require('url'); const assert = require('internal/assert'); @@ -190,6 +191,9 @@ function ClientRequest(input, options, cb) { this._ended = false; this.res = null; + // The aborted property is for backwards compat and is not + // strictly the same as destroyed as it can be written + // to by the user. this.aborted = false; this.timeoutCb = null; this.upgradeOrConnect = false; @@ -197,7 +201,14 @@ function ClientRequest(input, options, cb) { this.maxHeadersCount = null; this.reusedSocket = false; - let called = false; + // Used by destroyImpl. + this[kWritableState] = { + errorEmitted: false, + destroyed: false, + emitClose: true, + socketPending: true, + destroyCallback: null + }; if (this.agent) { // If there is an agent we should default to Connection:keep-alive, @@ -261,11 +272,17 @@ function ClientRequest(input, options, cb) { } const oncreate = (err, socket) => { - if (called) + const state = this[kWritableState]; + if (!state.socketPending) return; - called = true; + state.socketPending = false; if (err) { - process.nextTick(() => this.emit('error', err)); + const { destroyCallback: cb } = state; + if (cb) { + cb(err); + } else { + this.destroy(err); + } return; } this.onSocket(socket); @@ -280,9 +297,10 @@ function ClientRequest(input, options, cb) { this._last = true; this.shouldKeepAlive = false; if (typeof options.createConnection === 'function') { + const state = this[kWritableState]; const newSocket = options.createConnection(options, oncreate); - if (newSocket && !called) { - called = true; + if (newSocket && state.socketPending) { + state.socketPending = false; this.onSocket(newSocket); } else { return; @@ -298,6 +316,12 @@ function ClientRequest(input, options, cb) { Object.setPrototypeOf(ClientRequest.prototype, OutgoingMessage.prototype); Object.setPrototypeOf(ClientRequest, OutgoingMessage); +Object.defineProperty(ClientRequest.prototype, 'destroyed', { + get() { + return this[kWritableState].destroyed; + } +}); + ClientRequest.prototype._finish = function _finish() { DTRACE_HTTP_CLIENT_REQUEST(this, this.socket); OutgoingMessage.prototype._finish.call(this); @@ -311,28 +335,37 @@ ClientRequest.prototype._implicitHeader = function _implicitHeader() { this[kOutHeaders]); }; -ClientRequest.prototype.abort = function abort() { - if (!this.aborted) { - process.nextTick(emitAbortNT.bind(this)); - } +ClientRequest.prototype.destroy = destroy; +ClientRequest.prototype._destroy = function(err, cb) { this.aborted = true; + process.nextTick(emitAbortNT, this); // If we're aborting, we don't care about any more response data. if (this.res) { this.res._dump(); } - // In the event that we don't have a socket, we will pop out of - // the request queue through handling in onSocket. - if (this.socket) { + if (this.upgradeOrConnect) { + // We're detached from socket. + cb(err); + } else if (this.socket) { // in-progress - this.socket.destroy(); + this.socket.destroy(err, cb); + } else if (this[kWritableState].socketPending) { + // In the event that we don't have a socket, we will pop out of + // the request queue through handling in onSocket. + this[kWritableState].destroyCallback = (er) => cb(er || err); + } else { + cb(err); } }; +ClientRequest.prototype.abort = function abort() { + this.destroy(); +}; -function emitAbortNT() { - this.emit('abort'); +function emitAbortNT(self) { + self.emit('abort'); } function ondrain() { @@ -363,24 +396,23 @@ function socketCloseListener() { res.aborted = true; res.emit('aborted'); } - req.emit('close'); if (!res.aborted && res.readable) { res.on('end', function() { + // We can only destroy req after 'end'. Otherwise we will dump the + // data. + req.destroy(); this.emit('close'); }); res.push(null); } else { + req.destroy(); res.emit('close'); } } else { - if (!req.socket._hadError) { - // This socket error fired before we started to - // receive a response. The error needs to - // fire on the request. - req.socket._hadError = true; - req.emit('error', connResetException('socket hang up')); - } - req.emit('close'); + // This socket error fired before we started to + // receive a response. The error needs to + // fire on the request. + req.destroy(connResetException('socket hang up')); } // Too bad. That output wasn't getting written. @@ -400,13 +432,6 @@ function socketErrorListener(err) { const req = socket._httpMessage; debug('SOCKET ERROR:', err.message, err.stack); - if (req) { - // For Safety. Some additional errors might fire later on - // and we need to make sure we don't double-fire the error event. - req.socket._hadError = true; - req.emit('error', err); - } - const parser = socket.parser; if (parser) { parser.finish(); @@ -416,7 +441,7 @@ function socketErrorListener(err) { // Ensure that no further data will come out of the socket socket.removeListener('data', socketOnData); socket.removeListener('end', socketOnEnd); - socket.destroy(); + req.destroy(err); } function freeSocketErrorListener(err) { @@ -431,17 +456,15 @@ function socketOnEnd() { const req = this._httpMessage; const parser = this.parser; - if (!req.res && !req.socket._hadError) { - // If we don't have a response then we know that the socket - // ended prematurely and we need to emit an error on the request. - req.socket._hadError = true; - req.emit('error', connResetException('socket hang up')); - } if (parser) { parser.finish(); freeParser(parser, req, socket); } - socket.destroy(); + + // If we don't have a response then we know that the socket + // ended prematurely and we need to emit an error on the request. + const err = !req.res ? connResetException('socket hang up') : null; + req.destroy(err); } function socketOnData(d) { @@ -456,9 +479,7 @@ function socketOnData(d) { prepareError(ret, parser, d); debug('parse error', ret); freeParser(parser, req, socket); - socket.destroy(); - req.socket._hadError = true; - req.emit('error', ret); + req.destroy(ret); } else if (parser.incoming && parser.incoming.upgrade) { // Upgrade (if status code 101) or CONNECT const bytesParsed = ret; @@ -490,10 +511,10 @@ function socketOnData(d) { socket.readableFlowing = null; req.emit(eventName, res, socket, bodyHead); - req.emit('close'); + req.destroy(); } else { // Requested Upgrade or used CONNECT method, but have no handler. - socket.destroy(); + req.destroy(); } } else if (parser.incoming && parser.incoming.complete && // When the status code is informational (100, 102-199), @@ -582,7 +603,7 @@ function parserOnIncomingClient(res, shouldKeepAlive) { // If the user did not listen for the 'response' event, then they // can't possibly read the data, so we ._dump() it into the void // so that the socket doesn't hang there in a paused state. - if (req.aborted || !req.emit('response', res)) + if (req.destroyed || !req.emit('response', res)) res._dump(); if (method === 'HEAD') @@ -720,10 +741,11 @@ ClientRequest.prototype.onSocket = function onSocket(socket) { }; function onSocketNT(req, socket) { - if (req.aborted) { + if (req.destroyed) { + const { destroyCallback: cb } = req[kWritableState]; // If we were aborted while waiting for a socket, skip the whole thing. if (!req.agent) { - socket.destroy(); + socket.destroy(null, cb); } else { req.emit('close'); socket.emit('free'); diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index ded75b255934c4..7be8716753f323 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -1,12 +1,15 @@ 'use strict'; +const kWritableState = Symbol('kWritableState'); +const kReadableState = Symbol('kReadableState'); + function needError(stream, err) { if (!err) { return false; } - const r = stream._readableState; - const w = stream._writableState; + const r = stream[kReadableState] || stream._readableState; + const w = stream[kWritableState] || stream._writableState; if ((w && w.errorEmitted) || (r && r.errorEmitted)) { return false; @@ -25,8 +28,8 @@ function needError(stream, err) { // Undocumented cb() API, needed for core, not for public API. // The cb() will be invoked synchronously if _destroy is synchronous. function destroy(err, cb) { - const r = this._readableState; - const w = this._writableState; + const r = this[kReadableState] || this._readableState; + const w = this[kWritableState] || this._writableState; if (w && err) { w.errored = true; @@ -85,8 +88,8 @@ function emitErrorNT(self, err) { } function undestroy() { - const r = this._readableState; - const w = this._writableState; + const r = this[kReadableState] || this._readableState; + const w = this[kWritableState] || this._writableState; if (r) { r.destroyed = false; @@ -115,8 +118,8 @@ function errorOrDestroy(stream, err) { // the error to be emitted nextTick. In a future // semver major update we should change the default to this. - const r = stream._readableState; - const w = stream._writableState; + const r = stream[kReadableState] || stream._readableState; + const w = stream[kWritableState] || stream._writableState; if (w & err) { w.errored = true; @@ -132,5 +135,7 @@ function errorOrDestroy(stream, err) { module.exports = { destroy, undestroy, - errorOrDestroy + errorOrDestroy, + kWritableState, + kReadableState }; diff --git a/test/parallel/test-http-agent-uninitialized-with-handle.js b/test/parallel/test-http-agent-uninitialized-with-handle.js index 77f01771734c87..93765c858c6a77 100644 --- a/test/parallel/test-http-agent-uninitialized-with-handle.js +++ b/test/parallel/test-http-agent-uninitialized-with-handle.js @@ -12,6 +12,7 @@ const socket = new net.Socket(); socket._handle = { ref() { }, readStart() { }, + close() {} }; const server = http.createServer(common.mustCall((req, res) => { diff --git a/test/parallel/test-http-client-aborted.js b/test/parallel/test-http-client-aborted.js new file mode 100644 index 00000000000000..5a9f68a93a0e3e --- /dev/null +++ b/test/parallel/test-http-client-aborted.js @@ -0,0 +1,57 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const assert = require('assert'); +const EventEmitter = require('events') + +{ + const server = http.createServer(common.mustCall(function(req, res) { + req.on('aborted', common.mustCall(function() { + assert.strictEqual(this.aborted, true); + server.close(); + })); + assert.strictEqual(req.aborted, false); + res.write('hello'); + })); + + server.listen(0, common.mustCall(() => { + const req = http.get({ + port: server.address().port, + headers: { connection: 'keep-alive' } + }, common.mustCall((res) => { + req.destroy(new Error('kaboom')); + req.on('error', common.mustCall()); + assert.strictEqual(req.destroyed, true); + assert.strictEqual(req.aborted, true); + })); + })); +} + +{ + const req = http.request({ + createConnection: (_, callback) => { + process.nextTick(callback, new Error('kaboom')); + } + }); + req.on('socket', common.mustNotCall()); + req.destroy(); + req.on('error', common.mustCall(common.expectsError({ + message: 'kaboom' + }))); +} + +{ + // Make sure socket is destroyed if req.destroy + // is called before socket is ready. + const dummySocket = new EventEmitter(); + dummySocket.destroy = common.mustCall(); + dummySocket.on('free', common.mustNotCall()); + const req = http.request({ + createConnection: common.mustCall((_, callback) => { + process.nextTick(callback, null, dummySocket); + }) + }); + req.on('socket', common.mustNotCall()); + req.destroy(); +} diff --git a/test/parallel/test-http-client-close-event.js b/test/parallel/test-http-client-close-event.js index 7573931ac48ef6..4bd0832e4cfb6f 100644 --- a/test/parallel/test-http-client-close-event.js +++ b/test/parallel/test-http-client-close-event.js @@ -26,5 +26,7 @@ server.listen(0, common.mustCall(() => { server.close(); })); - req.destroy(); + const err = new Error('socket hang up'); + err.code = 'ECONNRESET'; + req.destroy(err); })); diff --git a/test/parallel/test-http-client-set-timeout.js b/test/parallel/test-http-client-set-timeout.js index 15aae7023bf3f1..9d267de6ac8dca 100644 --- a/test/parallel/test-http-client-set-timeout.js +++ b/test/parallel/test-http-client-set-timeout.js @@ -43,6 +43,8 @@ server.listen(0, mustCall(() => { req.on('timeout', mustCall(() => { strictEqual(req.socket.listenerCount('timeout'), 0); - req.destroy(); + const err = new Error('socket hang up'); + err.code = 'ECONNRESET'; + req.destroy(err); })); }));