Skip to content

Make clientError overridable #4557

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.

Sign up for GitHub

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

Closed
indutny wants to merge 5 commits into from
Closed

Make clientError overridable #4557

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0f65247
tls: rename `clientError` to `tlsClientError`
indutny Jan 6, 2016
9fc35f4
http: overridable `clientError`
indutny Jan 6, 2016
3967df7
doc: http fixes
indutny Jan 6, 2016
5515479
http: test and fixes
indutny Jan 7, 2016
2ba3898
https: fixes
indutny Jan 7, 2016
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
Prev Previous commit
Next Next commit
http: overridable clientError 
Make default `clientError` behavior (close socket immediately)
overridable. With this APIs it is possible to write a custom error
handler, and to send, for example, a 400 HTTP response.

    http.createServer(...).on('clientError', function(err, socket) {
      socket.end('HTTP/1.1 400 Bad Request\r\n\r\n');
      socket.destroy();
    });

Fix: #4543
  • Loading branch information
@indutny
indutny committed Jan 6, 2016
commit 9fc35f4e9351efdad55db53f571cc98b7ee8c92a
4 changes: 4 additions & 0 deletions doc/api/http.markdown
View file
Open in desktop
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

@mscdex mscdex Jan 6, 2016

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

@indutny indutny Jan 6, 2016

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

@mscdex mscdex Jan 6, 2016

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."

Copy link

@triblondon triblondon Jan 6, 2016

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

@indutny indutny Jan 6, 2016

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
7 changes: 2 additions & 5 deletions lib/_http_server.js
View file
Open in desktop
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
5 changes: 3 additions & 2 deletions lib/https.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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