search

expressjs / express

Fast, unopinionated, minimalist web framework for node.
https://expressjs.com
MIT License
65.78k stars 16.42k forks source link

Trailer header field included with 304 #2749

Open ghost opened 9 years ago

ghost commented 9 years ago

Including the Trailer header field with responses that don't have Transfer-Encoding: chunked causes some (overly strict?) proxies to drop the response e.g. IBM Bluemix sends back a 502 instead of the response generated by Express.

I think a possible solution here would be to remove the Trailer header field for 304, in addition to the fields currently removed. Here is where that code lives: https://github.com/strongloop/express/blob/f73ff9243006ea010fffdaa748f06df3a5b986e7/lib/response.js#L192

ghost commented 9 years ago

Would it make sense to also move trailer values to the header of the 304 response here-ish?

dougwilson commented 9 years ago

Would it make sense to also move trailer values to the header of the 304 response here-ish?

By their nature, the trailing values won't even be available until after the application has finished sending the response body (since, for example, they may be calculating a MD5 hash).

Right now, the fact that you can write a body in a 304 and it doesn't actually get sent is all handled by Node.js core HTTP. Perhaps that may be the better place to add this functionality?

ghost commented 9 years ago

Thanks! You're of course right and the trailer won't be known until the response finishes streaming, which defeats the purpose.

I'll open an issue for Node about Trailer.

hacksparrow commented 8 years ago

@dougwilson @wprl so can we close this?

dougwilson commented 8 years ago

This is actually a bug. Changing the labels.

tunniclm commented 8 years ago

Is sounds like the spec allows a response to have a Trailer: ... header without the matching trailer. It sounds to me like the proxy should be allowing this valid response through.

The user (ie app/middleware author) could also prevent this issue by not adding a Trailer: ... header to responses that are not chunked (including 304s which shouldn't be chunked).

Is there really a bug in Express to fix here?

EDIT: Prematurely hit "Comment", added the rest in via an edit.

dougwilson commented 8 years ago

Yea, @tunniclm, I sort of agree :) You see, by default Node.js will send responses chunked, but the Express helpers actually force them to non-chunked, which is a more efficient data transfer mechanism and easier to parse for a lot of clients. Ideally Express needs to be a lot smarter around this and how it decides to send the response out according to the headers that were built up before writing them.

tunniclm commented 8 years ago

Do you still think this is a bug, or is it something that can be addressed as an enhancement? And what are you proposing we do? If we can define the goal of a change more explicitly, I'm happy to have a go at making it.