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 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/api/http.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,10 @@ 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 destroying socket, and, for example,
can do it it gracefully by sending 400 HTTP response.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe re-word this to something like:

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.


Default behavior is destroy socket immediately.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps: "Default behavior is to destroy the socket immediately."

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For people searching the page, mentioning "malformed request" or "invalid HTTP" would be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.


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

Expand Down
2 changes: 1 addition & 1 deletion doc/api/tls.markdown
Original file line number Diff line number Diff line change
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
7 changes: 2 additions & 5 deletions lib/_http_server.js
Original file line number Diff line number Diff line change
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,8 @@ function connectionListener(socket) {

// TODO(isaacs): Move all these functions out of here
function socketOnError(e) {
self.emit('clientError', e, this);
if (!self.emit('clientError', e, this))
this.destroy(e);
}

function socketOnData(d) {
Expand Down
4 changes: 2 additions & 2 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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();
});

this.timeout = 2 * 60 * 1000;
Expand Down
6 changes: 0 additions & 6 deletions test/parallel/test-http-client-abort.js
Original file line number Diff line number Diff line change
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
1 change: 1 addition & 0 deletions test/parallel/test-https-timeout-server.js
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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