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

Don't send a Content-Length header for 204 (No Data) responses. #11

Merged
merged 2 commits into from
Apr 11, 2016

Conversation

trasch
Copy link
Contributor

@trasch trasch commented Apr 8, 2016

Some clients are quite picky about receiving 204 responses with a content-length header. This patch solves this by explicitly setting the Content-Length header to 0 for such responses (i.e. empty tiles).

@@ -230,7 +230,7 @@ TileServer.prototype.serve = function(req, http, callback) {
},
function finalizeResult(callback) {
if (!result.headers) result.headers = {};
result.headers['Content-Length'] = result.buffer ? result.buffer.length : 0;
result.headers['Content-Length'] = result.buffer && result.status !== 204 ? result.buffer.length : 0;
Copy link
Member

Choose a reason for hiding this comment

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

In these cases won't the buffer be empty, which would lead to a Content-Length of 0 already? If we're not wanting to send the header at all, wouldn't it need to be something more like this?

if (result.status !== 204) {
    result.headers['Content-Length'] = result.buffer.length;
}

@BHare1985
Copy link

According to the RFC 7230 spec section 3.3.2 on content-length

A server MUST NOT send a Content-Length header field in any response
with a status code of 1xx (Informational) or 204 (No Content).

Therefore the content-length header should not even appear. Anyone who relies on a content-length for a 204 HTTP status code is wrong. Judging by the title of this PR, I think brian's code suggestion is what you intended.

@trasch
Copy link
Contributor Author

trasch commented Apr 11, 2016

Right, I updated the patch accordingly.

@brianreavis brianreavis merged commit f7f01c8 into naturalatlas:master Apr 11, 2016
@brianreavis
Copy link
Member

Thanks @trasch!

@brianreavis
Copy link
Member

This is now out: 2.0.2

@trasch
Copy link
Contributor Author

trasch commented Apr 12, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants