2.1.2 Breaks my Stream Chain

[nullivex]

Hello,

I am working on releasing a new version and I ran into a strange error. My promise pipe would no longer return.

I have what I believe is a simple use case. I have also verified reverting to 2.1.1 fixes my issue. I prefer not to pin packages if possible.

I inspected this commit which I believe is the root of my issue: cb17b5c

Here is my usage

const P = require('bluebird')
const fs = require('fs')
const hashStream = require('sha1-stream')
const promisePipe = require('promisepipe')
const request = require('request')

let sniff = hashStream.createStream('sha1')
let writeStream = fs.createWriteStream('somefile')

P.try(function(){
  return promisePipe(request('http://www.google.com'),sniff,writeStream)
})
  .then(result => {console.log('got here',sniff.hash,result)})

NOTE I am using bluebird promises in my application and wanted to preserve that behavior in case it makes a difference.

It seems like with the new waiting for the final write stream to finish the new behavior has broken my use case.

Now I control the sha1-stream package so if I am making some kind of messaging mistake there I can fix that.

This seems like it should still work.

Thanks for any help.

[MartinKolarik]

Hi, the problem is caused by the request module. We check if the stream has a _read() method to determine whether it's a writable or a readable stream. According to nodejs docs "Custom Readable streams must call the new stream.Readable([options]) constructor and implement the readable._read() method."

The "stream" returned by request doesn't have that method, so we treat is as a writable stream and wait for a finish event that never comes. After searching request's issue tracker I'd say their stream API has quite a lot of issues and I'd recommend using got module instead (which I tested and it worked ok).

Given that this change was done in a patch release and can cause problems for some users (although it isn't this module's fault), I'm considering publishing 2.1.3 with code from 2.0.0 and then publishing the current code as 3.0.0. What do you think?

[nullivex]

@MartinKolarik that sounds very close to what I was theorizing.

That being said, I dont think my use case is out of place and we are not in a position to change the request package at the moment. So, is there a way to make the behavior switchable? Or should I pipe request to through2 perhaps and have it emulate a real stream?

UPDATE I experimented with piping to through2 it doesnt seem to alleviate the condition. For now I am going to leave this package pinned @2.1.1. Let me know how you are going to proceed. It would be nice for this module to be flexible and be able to talk to broken packages / streams when enabled.

[MartinKolarik]

Using through2 would fix this problem but also create a new one, since you'd have to manually add an error handler for the original request stream.

I could change the check from typeof stream._read === 'function' to typeof stream._read === 'function' || stream.readable which would make it work with request and possibly other modules, and hopefully wouldn't break anything. Not great, but I guess it's the best solution here to improve compatibility with other modules.

[MartinKolarik]

Fix published in 2.1.3.

[nullivex]

@MartinKolarik thank you so much for the quick turnaround! Amazing.

I tested 2.1.3 just now and can confirm the issue has been resolved!

Thanks again 👍