Add setEncoding() support to zlib streams #3512

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@mhart
mhart commented Jun 22, 2012

No description provided.

@mhart
mhart commented Jun 22, 2012

I also think it's a bit unfortunate that the API docs seem to suggest that any readable Stream supports setEncoding() - whereas this is not true - a stream needs to implement it manually (and the Zlib stream hasn't as yet).

@bnoordhuis
Member

@isaacs You should probably review this.

The patch LGTM but I don't like the concept. That's mainly because I don't like the concept of .setEncoding(), it should never have been part of the Stream interface to start with.

@isaacs
isaacs commented Jun 22, 2012

I have to agree with @bnoordhuis here. setEncoding is kind of an awful kludge. That being said, I'm kind of not willing to make a decision on this just yet. Let's sit on it for a bit.

We're going to clean up the Stream API a bit in v0.9. Either setEncoding will be blessed, or removed from the Stream interface (though streams that already have that method will keep it.) Depending on the results of that investigation, this patch may or may not land.

@mhart
mhart commented Jun 23, 2012

I agree that setEncoding looks pretty messy at the moment - it feels like the right way to handle it would be to have a StringStream or some such that you pipe your data stream into. If such a thing was created, you could always keep the existing setEncoding behaviour by using the piped stream behind the scenes (happy to create a pull request if you like).

This issue I have at the moment is that this method is supported in fs, http, net, readline and tls - and the API docs. Why zlib got short shrift doesn't seem to make much sense. And it certainly makes writing filesystem code a lot messier if you want to deal with .gz files transparently, ie:

function processFile(file) {
  var stream = fs.openReadStream(file);
  if (/\.gz$/.test(file)) stream = stream.pipe(zlib.createGunzip());
  stream.setEncoding('utf8'); // Can't do this!!!

  stream.on('data', str) {
    // process str here
  }
}

I wanted to add support for this recently for node-bunyan - and you can see the hacky way I had to resort to doing it, by using the same internal decoder method the other streams do:

https://github.com/trentm/node-bunyan/pull/24/files#L0R700

@mhart
mhart commented Jun 23, 2012

Another issue I've run into with setEncoding for example is that StringDecoder doesn't deal with base64 correctly when it's coming in a stream - if a mid-stream buffer length is not divisible by 3, then you end up with == characters mid-stream.

I've been working on a patch for this that just buffers data in the StringDecoder and emits the base64 string only when it's aligned - it does require the stream to notify the StringDecoder when it's finished so that the final few bytes (if any) can be flushed - so this aspect requires a slight addition to each stream if it currently supports using a StringDecoder.

I was going to try and submit a pull request for this - but if you're saying that the whole setEncoding aspect is on hold, then there might not be much point. If a StringStream pipe existed, it would certainly be better suited to that...

Doesn't stop the fact that the current setEncoding + base64 is broken/unusable though unfortunately.

@mhart
mhart commented Jun 23, 2012

OK, I've created a module for StringStream - also deals with the base64 issue correctly:

https://github.com/mhart/StringStream

@isaacs
isaacs commented Jun 23, 2012

Why zlib got short shrift doesn't seem to make much sense.

The reason is that node's zlib binding is much younger than those others. It was introduced in 0.6, they all date back to 0.2. In 0.2, there was no such thing as Buffers, so you had to tell streams what sort of string to give you.

@Nodejs-Jenkins

Can one of the admins verify this patch?

@mhart
mhart commented Mar 13, 2013

A quick glance at the new streams API seems to suggest that setEncoding should be supported on all readable streams - but it would be good to run the test in the attached test-zlib-encoding.js to ensure it acts in the expected way.

@mhart
mhart commented Mar 14, 2013

I've just tested this, it works fine in v0.10.0. Closing this pull request.

@mhart mhart closed this Mar 14, 2013
@mhart mhart deleted the mhart:zlib-encoding-support branch Mar 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment