Skip to content

Conversation

@jsdevel
Copy link

@jsdevel jsdevel commented Jun 22, 2014

No description provided.

@dougwilson
Copy link
Contributor

204 is not an error code. next(err) is meant for errors only, which is >= 400.

@dougwilson dougwilson closed this Jun 22, 2014
@jsdevel
Copy link
Author

jsdevel commented Jun 22, 2014

@dougwilson makes sense. The existing code honors it though so perhaps it should be updated to barf on err.status < 400?

@dougwilson
Copy link
Contributor

Ideally, but it's too late now until the next major. Also, I'm not sure what this is trying to accomplish, because Node.js core will automatically discard the body when you set the status to 204 (see https://github.com/joyent/node/blob/v0.10.29/lib/http.js#L1181-L1194 and https://github.com/joyent/node/blob/v0.10.29/lib/http.js#L844 and https://github.com/joyent/node/blob/v0.10.29/lib/http.js#L921).

@jsdevel
Copy link
Author

jsdevel commented Jun 22, 2014

@dougwilson strangely enough the response was never closed using request.js. It's probably worth further investigation to see if it's an issue in either node core or request.js.

@dougwilson
Copy link
Contributor

strangely enough the response was never closed using request.js

I'm not sure what that means. When I applied your PR without the lib/application.js change, it still passed, so it doesn't seem like that test you added even relied on the change you made to the error handler.

@jsdevel jsdevel deleted the handling-204-status-code-in-error branch June 22, 2014 04:21
@jsdevel
Copy link
Author

jsdevel commented Jun 22, 2014

Here you go @dougwilson pillarjs/finalhandler#1. Thanks for the direction!

@expressjs expressjs locked and limited conversation to collaborators Jul 18, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants