In http.Agent, response.readable is never set to false, causing bugs in response.pipe() #867
Conversation
| this.output += buffer.toString(); | ||
| console.log('wrote data.'); | ||
| var self = this; | ||
| this.emit('pause'); |
There was a problem hiding this comment.
You shouldn't need to emit pause on a writable stream. Returning false from the write function will in turn make the read stream call pause on itself. See the Stream#pipe() source to see what I'm talking about.
There was a problem hiding this comment.
Thanks for pointing that out. Updated to return false instead of emitting 'pause'. This doesn't change the behavior of the test case in demonstrating the bug.
…ait for drain event
|
The test isn't failing for me before the fix: https://gist.github.com/914705 |
|
Interesting. I re-tested with the latest code and found that it was no longer failing for me either. After digging it turns out that 2a65d2962578db2cc841 fixed the bug that was causing an error in pipe() - now all pipe()-related event handlers get removed as soon as the input ends. Previously, source.resume() could be called after the source emitted 'end', which was a bug. So my test case no longer fails. Still, it seems that res.readable should be set to false at the time of the 'end' event, since attempting to read from it after that point will cause an error. I've updated this pull request with a simpler test case that no longer uses pipe(). I've also accidentally pulled a merge into this pull request, which I can't figure out how to fix at the moment. Please ignore that and just look at the diff. I can make a new pull request if necessary. Finally, I'm wondering if it would be preferable to set readable = false at a lower level (like around line 132), but I don't have a solid enough grasp of the code to be sure of the implications of doing it that way. |
|
Thanks. Landed in 83727a4 |
|
since this has landed, can someone close the pull request? |
I was seeing an intermittent bug in my app Listening Room when piping an http response into stdin of a process. After digging around I found the bug - response.readable was never getting set to false. So if the response finished while the destination stream was paused, when the destination stream resumed pipe() would call source.resume(), which would throw an error since response.socket didn't exist any more.
The attached test demonstrates the bug - run the test case on the existing code and it will throw an error, apply my patch and it works.