HttpServerResponse.setDefaultEncoding()

[shaunc]

• Version: 7.10.0
• Platform: mac
• Subsystem: http

The documentation of http.ServerResponse claims that it implements the interface of stream.Writable, which includes setDefaultEncoding. However, ServerResponse does not implement setDefaultEncoding.

I'm passing a server response to another library, which pipes an archiver.zip to it. The binary data is interpreted as utf8. I should be able to avoid this by setting the default encoding.

[TimothyGu]

Nor does ServerResponse or OutgoingMessage implement cork or uncork...

[joaolucasl]

If it makes sense to implement these non-existent functions both for ServerResponse and for OutgoingMessage, I'd like to take a crack at it!

From my understanding, something in the lines of util.inherits(ServerResponse, stream.Writable); would suffice?

[soletan]

@joaolucasl Guess this simple line won't suffice according to this note in docs on Class: http.ServerResponse:

The response implements, but does not inherit from, the Writable Stream interface. This is an EventEmitter with the following events:

Nonetheless, this statement is wrong since setDefaultEncoding(), cork() and uncork() are part of Writable interface.

Aside from that I second this issue. Any fix might help me with issues I'm facing when piping from same source into ServerResponse and into fs.WriteStream with the former generating garbage (with some more bytes sent) and the latter resulting in valid output.

EDIT: This fix would be useful in LTS release, too.

[mick-io]

I'd be happy to work on this issue this weekend.

[boneskull]

I was looking at this, and can confirm there's "extra bytes" when piping a readable stream into e.g., a ServerResponse and process.stdout (to @soletan's) comment.

This seems to be because Transfer-encoding: chunked (wiki) is set by default (for response codes other than 204 and 304, anyway).

To avoid this you can use e.g., serverResponse.removeHeader('transfer-encoding').

I can't reproduce a situation in which a ReadableStream containing a Buffer piped into a ServerResponse inexplicably becomes a string.

The headers are text, of course--and it's all the same stream--so I can only guess that whatever @shaunc is using may be confused by this?

If I'm right, then this issue should be closed, as setDefaultEncoding() isn't the problem. @apapirovski what do you think?

[ghost]

Interesting Discussion! and I agree with @soletan：

I took a very close look at the source codes, found that:

For ServerResponse, we've inherited from OutgoingMessage, which DOESN'T inherit from Stream.Writable but Stream ONLY!

[_http_outgoing.js]

function OutgoingMessage() {
  Stream.call(this);
  ……
}
util.inherits(OutgoingMessage, Stream);
……

[_http_server.js]

util.inherits(ServerResponse, OutgoingMessage);

That's the reason why we cannot implement cork(), uncork()……ect (Is it on purpose?)

But if this is changed to implement from Stream.Writable, maybe a lot of related things are changed, too, with a major version modified.

Considering this discussion has lasted for long ages without a confirmed result, and in order to make it clear, we have to invite related groups to have a discussion on it to see whether we should change the doc or codes there?

/cc：@nodejs/streams, @nodejs/http.

[mcollina]

I have tried several times to make OutgoingMessage a direct descendant of Stream.Writable. Unfortunately, doing so would cause a significant (X0%) degradation of throughput for HTTP servers. I don't think that's acceptable. We end up doing this because otherwise we would have double-buffering in OutgoingMessage  and the Socket, which is costly.

The solution is to implement all methods in OutgoingMessage, or maybe create a WrappedWritable class that is shared between HTTP, HTTP2 and possibly others. This seems a pretty common case.

[ghost]

@mcollina：Thanks for sharing ideas. And I see the post is lasting for ages with help wanted. So this is the reason why I invite groups here to tell the reasons of your thinkings :)

And to be honest：Because each request will create a new instance of ServerResponse, so we cannot use this method to re-use the default encoding, so write's encoding assignment is OK with me instead of setDefaultEncoding, generally we cannot re-use the same ServerResponse instance again and again, so it seems setDefaultEncoding is a little duplicated :)

BTW：Until now the doc says (as what @soletan says above)：

The response implements, but does not inherit from, the Writable Stream interface. This is an EventEmitter with the following events……

Is it necessary to change the explaination of doc (because we haven't implemented cork(), uncork()……).

I'd tend to change this to:

The response inherits from [Stream][], with some methods like what we have in [Writable Stream][]. There's an EventEmitter with the following events……

If it's OK, I'd help to submit another PR for that :)

[mcollina]

Definitely. Also, make the same amendment to the HTTP2 docs.

[ghost]

@mcollina ：Thanks for your quick resp! I'll do that :)

[gireeshpunathil]

can this be closed, given #22305 is landed?

[jasnell]

Closing as it appears this is resolved. Can reopen if it turns out that it's not.