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

Conversation

indutny
Copy link
Member

@indutny indutny commented Jan 6, 2016

See: #4543

`clientError` will have `http.Server`-specific behavior, and we don't
want to shadow it in `tls.Server`.
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: nodejs#4543
@indutny
Copy link
Member Author

indutny commented Jan 6, 2016

cc @nodejs/http

Listener of this event is responsible for destroying socket, and, for example,
can do it it gracefully by sending 400 HTTP response.

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.

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 6, 2016
@vkurchatkin vkurchatkin removed http Issues or PRs related to the http subsystem. tls Issues and PRs related to the tls subsystem. labels Jan 6, 2016
@mscdex mscdex added tls Issues and PRs related to the tls subsystem. http Issues or PRs related to the http subsystem. labels Jan 6, 2016
@vkurchatkin vkurchatkin added tls Issues and PRs related to the tls subsystem. and removed tls Issues and PRs related to the tls subsystem. labels Jan 6, 2016
@Trott
Copy link
Member

Trott commented Jan 6, 2016

Should there be a test that triggers clientError with and without the override to confirm both more-or-less basically work as expected? Or is the idea that the modified TLS tests already check the override and other existing tests already check the basic behavior?

@indutny
Copy link
Member Author

indutny commented Jan 6, 2016

@Trott yep, I will write a test. Thank you for suggestion

@indutny
Copy link
Member Author

indutny commented Jan 7, 2016

@Trott how about this? ;)

@indutny
Copy link
Member Author

indutny commented Jan 7, 2016

@mscdex PTAL

@indutny
Copy link
Member Author

indutny commented Jan 7, 2016

@mscdex
Copy link
Contributor

mscdex commented Jan 7, 2016

LGTM if CI is ok with it.

@indutny
Copy link
Member Author

indutny commented Jan 7, 2016

Landed in 1ab6b21 and 5f76b24, thank you!

indutny added a commit that referenced this pull request Jan 7, 2016
`clientError` will have `http.Server`-specific behavior, and we don't
want to shadow it in `tls.Server`.

PR-URL: #4557
Reviewed-By: Brian White <mscdex@mscdex.net>
indutny added a commit that referenced this pull request Jan 7, 2016
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
PR-URL: #4557
Reviewed-By: Brian White <mscdex@mscdex.net>
@indutny indutny closed this Jan 7, 2016
@indutny indutny deleted the fix/gh-4543 branch January 7, 2016 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants