Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make clientError overridable #4557

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/api/http.markdown
Expand Up @@ -421,6 +421,11 @@ not be emitted.
`function (exception, socket) { }`

If a client connection emits an `'error'` event, it will be forwarded here.
Listener of this event is responsible for closing/destroying the underlying
socket. For example, one may wish to more gracefully close the socket with an
HTTP '400 Bad Request' response instead of abruptly severing the connection.

Default behavior is to destroy the socket immediately on malformed request.

`socket` is the [`net.Socket`][] object that the error originated from.

Expand Down
2 changes: 1 addition & 1 deletion doc/api/tls.markdown
Expand Up @@ -169,7 +169,7 @@ This class is a subclass of `net.Server` and has the same methods on it.
Instead of accepting just raw TCP connections, this accepts encrypted
connections using TLS or SSL.

### Event: 'clientError'
### Event: 'tlsClientError'

`function (exception, tlsSocket) { }`

Expand Down
15 changes: 8 additions & 7 deletions lib/_http_server.js
Expand Up @@ -237,10 +237,6 @@ function Server(requestListener) {

this.addListener('connection', connectionListener);

this.addListener('clientError', function(err, conn) {
conn.destroy(err);
});

this.timeout = 2 * 60 * 1000;

this._pendingResponseData = 0;
Expand Down Expand Up @@ -353,7 +349,12 @@ function connectionListener(socket) {

// TODO(isaacs): Move all these functions out of here
function socketOnError(e) {
self.emit('clientError', e, this);
// Ignore further errors
this.removeListener('error', socketOnError);
this.on('error', () => {});

if (!self.emit('clientError', e, this))
this.destroy(e);
}

function socketOnData(d) {
Expand All @@ -372,7 +373,7 @@ function connectionListener(socket) {
function onParserExecuteCommon(ret, d) {
if (ret instanceof Error) {
debug('parse error');
socket.destroy(ret);
socketOnError.call(socket, ret);
} else if (parser.incoming && parser.incoming.upgrade) {
// Upgrade or CONNECT
var bytesParsed = ret;
Expand Down Expand Up @@ -418,7 +419,7 @@ function connectionListener(socket) {

if (ret instanceof Error) {
debug('parse error');
socket.destroy(ret);
socketOnError.call(socket, ret);
return;
}

Expand Down
4 changes: 2 additions & 2 deletions lib/_tls_wrap.js
Expand Up @@ -809,14 +809,14 @@ function Server(/* [options], listener */) {
errorEmitted = true;
var connReset = new Error('socket hang up');
connReset.code = 'ECONNRESET';
self.emit('clientError', connReset, socket);
self.emit('tlsClientError', connReset, socket);
}
});

socket.on('_tlsError', function(err) {
if (!socket._controlReleased && !errorEmitted) {
errorEmitted = true;
self.emit('clientError', err, socket);
self.emit('tlsClientError', err, socket);
}
});
});
Expand Down
5 changes: 3 additions & 2 deletions lib/https.js
Expand Up @@ -29,8 +29,9 @@ function Server(opts, requestListener) {
this.addListener('request', requestListener);
}

this.addListener('clientError', function(err, conn) {
conn.destroy();
this.addListener('tlsClientError', function(err, conn) {
if (!this.emit('clientError', err, conn))
conn.destroy(err);
});

this.timeout = 2 * 60 * 1000;
Expand Down
6 changes: 0 additions & 6 deletions test/parallel/test-http-client-abort.js
Expand Up @@ -21,12 +21,6 @@ var server = http.Server(function(req, res) {
server.close();
}
});

// since there is already clientError, maybe that would be appropriate,
// since "error" is magical
req.on('clientError', function() {
console.log('Got clientError');
});
});

var responses = 0;
Expand Down
39 changes: 39 additions & 0 deletions test/parallel/test-http-server-client-error.js
@@ -0,0 +1,39 @@
'use strict';
const common = require('../common');
const assert = require('assert');

const http = require('http');
const net = require('net');

const server = http.createServer(common.mustCall(function(req, res) {
res.end();
}));

server.on('clientError', common.mustCall(function(err, socket) {
socket.end('HTTP/1.1 400 Bad Request\r\n\r\n');

server.close();
}));

server.listen(common.PORT, function() {
function next() {
// Invalid request
const client = net.connect(common.PORT);
client.end('Oopsie-doopsie\r\n');

var chunks = '';
client.on('data', function(chunk) {
chunks += chunk;
});
client.once('end', function() {
assert.equal(chunks, 'HTTP/1.1 400 Bad Request\r\n\r\n');
});
}

// Normal request
http.get({ port: common.PORT, path: '/' }, function(res) {
assert.equal(res.statusCode, 200);
res.resume();
res.once('end', next);
});
});
1 change: 1 addition & 0 deletions test/parallel/test-https-timeout-server.js
Expand Up @@ -32,6 +32,7 @@ server.on('clientError', function(err, conn) {
assert.equal(conn._secureEstablished, false);
server.close();
clientErrors++;
conn.destroy();
});

server.listen(common.PORT, function() {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-tls-econnreset.js
Expand Up @@ -48,7 +48,7 @@ var connectError = null;

var server = tls.createServer({ ca: ca, cert: cert, key: key }, function(conn) {
throw 'unreachable';
}).on('clientError', function(err, conn) {
}).on('tlsClientError', function(err, conn) {
assert(!clientError && conn);
clientError = err;
}).listen(common.PORT, function() {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-tls-no-sslv3.js
Expand Up @@ -48,7 +48,7 @@ server.listen(common.PORT, '127.0.0.1', function() {
}));
});

server.on('clientError', err => errors.push(err));
server.on('tlsClientError', err => errors.push(err));

process.on('exit', function() {
if (/unknown option -ssl3/.test(stderr)) {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-tls-sni-option.js
Expand Up @@ -110,7 +110,7 @@ var server = tls.createServer(serverOptions, function(c) {
serverResults.push({ sni: c.servername, authorized: c.authorized });
});

server.on('clientError', function(err) {
server.on('tlsClientError', function(err) {
serverResults.push(null);
serverError = err.message;
});
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-tls-timeout-server.js
Expand Up @@ -25,7 +25,7 @@ var options = {

var server = tls.createServer(options, common.fail);

server.on('clientError', function(err, conn) {
server.on('tlsClientError', function(err, conn) {
conn.destroy();
server.close();
clientErrors++;
Expand Down