From 965889ee3b5ad9bcb5a3badefeb23b461da87966 Mon Sep 17 00:00:00 2001 From: Sven Slootweg Date: Thu, 9 Apr 2015 12:11:25 +0200 Subject: [PATCH] v1.1.1: Fix "old mode" stream error in Node v0.10, and replace .resume() calls with a pipe to the dev-null module (for 0.10 compatibility) --- lib/bhttp.coffee | 32 ++++++++++++++++++++++++++------ lib/bhttp.js | 32 +++++++++++++++++++++++++++----- package.json | 3 ++- test-bhttp.coffee | 15 +++++++++++++-- test-https-stream.coffee | 7 +++++++ test-progress.coffee | 3 ++- 6 files changed, 77 insertions(+), 15 deletions(-) create mode 100644 test-https-stream.coffee diff --git a/lib/bhttp.coffee b/lib/bhttp.coffee index 6c5a970..4dbfc44 100644 --- a/lib/bhttp.coffee +++ b/lib/bhttp.coffee @@ -21,6 +21,7 @@ debug = require("debug") debugRequest = debug("bhttp:request") debugResponse = debug("bhttp:response") extend = require "extend" +devNull = require "dev-null" # Other third-party modules formData = require "form-data2" @@ -452,7 +453,7 @@ processResponse = (request, response, requestState) -> else return redirectUnchanged request, response, requestState else if request.responseOptions.discardResponse - response.resume() # Drain the response stream + response.pipe(devNull()) # Drain the response stream Promise.resolve response else totalBytes = response.headers["content-length"] @@ -469,10 +470,29 @@ processResponse = (request, response, requestState) -> request.responseOptions.onDownloadProgress(completedBytes, totalBytes, response) new Promise (resolve, reject) -> - # This is slightly hacky, but returning a .pipe'd stream would make all the response object attributes unavailable to the end user. Therefore, 'branching' the stream into our progress monitoring stream is a much better option. We use .pause() to ensure the stream doesn't start flowing until the end user (or our library) has actually piped it into something. - response.pipe(progressStream) - response.pause() - + # This is a very, very dirty hack - however, using .pipe followed by .pause breaks in Node.js v0.10.35 with "Cannot switch to old mode now". Our solution is to monkeypatch the `on` and `resume` methods to attach the progress event handler as soon as something else is attached to the response stream (or when it is drained). This way, a user can also pipe the response in a later tick, without the stream draining prematurely. + _resume = response.resume.bind(response) + _on = response.on.bind(response) + _progressStreamAttached = false + + attachProgressStream = -> + # To keep this from sending us into an infinite loop. + if not _progressStreamAttached + debugResponse "attaching progress stream" + _progressStreamAttached = true + response.pipe(progressStream) + + response.on = (eventName, handler) -> + debugResponse "'on' called, #{eventName}" + if eventName == "data" or eventName == "readable" + attachProgressStream() + _on(eventName, handler) + + response.resume = -> + attachProgressStream() + _resume() + + # Continue with the regular response processing. if request.responseOptions.stream resolve response else @@ -527,7 +547,7 @@ redirectUnchanged = (request, response, requestState) -> doRedirect = (request, response, requestState, newOptions) -> Promise.try -> if not request.responseOptions.keepRedirectResponses - response.resume() # Let the response stream drain out... + response.pipe(devNull()) # Let the response stream drain out... requestState.redirectHistory.push response bhttpAPI._doRequest response.headers["location"], newOptions, requestState diff --git a/lib/bhttp.js b/lib/bhttp.js index 80c039d..862ee0f 100644 --- a/lib/bhttp.js +++ b/lib/bhttp.js @@ -1,4 +1,4 @@ -var Promise, S, addErrorData, bhttpAPI, bhttpErrors, concatStream, createCookieJar, debug, debugRequest, debugResponse, doPayloadRequest, doRedirect, errors, extend, formData, formFixArray, http, https, isStream, makeRequest, ofTypes, packageConfig, prepareCleanup, prepareDefaults, prepareOptions, preparePayload, prepareProtocol, prepareRequest, prepareSession, prepareUrl, processResponse, querystring, redirectGet, redirectUnchanged, sink, spy, stream, streamLength, toughCookie, urlUtil, util, _; +var Promise, S, addErrorData, bhttpAPI, bhttpErrors, concatStream, createCookieJar, debug, debugRequest, debugResponse, devNull, doPayloadRequest, doRedirect, errors, extend, formData, formFixArray, http, https, isStream, makeRequest, ofTypes, packageConfig, prepareCleanup, prepareDefaults, prepareOptions, preparePayload, prepareProtocol, prepareRequest, prepareSession, prepareUrl, processResponse, querystring, redirectGet, redirectUnchanged, sink, spy, stream, streamLength, toughCookie, urlUtil, util, _; urlUtil = require("url"); @@ -30,6 +30,8 @@ debugResponse = debug("bhttp:response"); extend = require("extend"); +devNull = require("dev-null"); + formData = require("form-data2"); concatStream = require("concat-stream"); @@ -487,7 +489,7 @@ processResponse = function(request, response, requestState) { } } } else if (request.responseOptions.discardResponse) { - response.resume(); + response.pipe(devNull()); return Promise.resolve(response); } else { totalBytes = response.headers["content-length"]; @@ -505,8 +507,28 @@ processResponse = function(request, response, requestState) { }); } return new Promise(function(resolve, reject) { - response.pipe(progressStream); - response.pause(); + var attachProgressStream, _on, _progressStreamAttached, _resume; + _resume = response.resume.bind(response); + _on = response.on.bind(response); + _progressStreamAttached = false; + attachProgressStream = function() { + if (!_progressStreamAttached) { + debugResponse("attaching progress stream"); + _progressStreamAttached = true; + return response.pipe(progressStream); + } + }; + response.on = function(eventName, handler) { + debugResponse("'on' called, " + eventName); + if (eventName === "data" || eventName === "readable") { + attachProgressStream(); + } + return _on(eventName, handler); + }; + response.resume = function() { + attachProgressStream(); + return _resume(); + }; if (request.responseOptions.stream) { return resolve(response); } else { @@ -573,7 +595,7 @@ redirectUnchanged = function(request, response, requestState) { doRedirect = function(request, response, requestState, newOptions) { return Promise["try"](function() { if (!request.responseOptions.keepRedirectResponses) { - response.resume(); + response.pipe(devNull()); } requestState.redirectHistory.push(response); return bhttpAPI._doRequest(response.headers["location"], newOptions, requestState); diff --git a/package.json b/package.json index ef54d44..018cdf6 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "bhttp", - "version": "1.1.0", + "version": "1.1.1", "description": "A sane HTTP client library for Node.js with Streams2 support.", "main": "index.js", "scripts": { @@ -25,6 +25,7 @@ "bluebird": "^2.8.2", "concat-stream": "^1.4.7", "debug": "^2.1.1", + "dev-null": "^0.1.1", "errors": "^0.2.0", "extend": "^2.0.0", "form-data2": "^1.0.0", diff --git a/test-bhttp.coffee b/test-bhttp.coffee index 1273033..6733d03 100644 --- a/test-bhttp.coffee +++ b/test-bhttp.coffee @@ -2,6 +2,7 @@ bhttp = require "./" Promise = require "bluebird" fs = require "fs" util = require "util" +devNull = require "dev-null" requestbinTarget = process.argv[2] if not requestbinTarget @@ -118,7 +119,7 @@ testcases.push(Promise.try -> ) testcases.push(Promise.try -> - # Cookie test + # No-redirect test session = bhttp.session() session.post "http://www.html-kit.com/tools/cookietester/", cn: "testkey1" @@ -128,10 +129,20 @@ testcases.push(Promise.try -> console.log "NO-REDIR", response.headers["location"] ) +testcases.push(Promise.try -> + # Redirect test + bhttp.get "http://google.com/" +.then (response) -> + console.log "REDIR", response.headers["location"], response.redirectHistory.length +) + Promise.all testcases .then -> bhttp.get "http://posttestserver.com/files/2015/01/19/f_03.01.01627545156", stream: true .then (response) -> #response.pipe process.stdout - response.resume() + response + .pipe(devNull()) + .on "finish", -> + console.log "GET STREAM", "ok" diff --git a/test-https-stream.coffee b/test-https-stream.coffee new file mode 100644 index 0000000..c0a3968 --- /dev/null +++ b/test-https-stream.coffee @@ -0,0 +1,7 @@ +bhttp = require "./" +Promise = require "bluebird" + +Promise.try -> + bhttp.get "https://google.com/" +.then (response) -> + console.log "Got response", response diff --git a/test-progress.coffee b/test-progress.coffee index 3074845..287c663 100644 --- a/test-progress.coffee +++ b/test-progress.coffee @@ -1,6 +1,7 @@ Promise = require "bluebird" bhttp = require "./" fs = require "fs" +devNull = require "dev-null" formatLine = (line) -> line.toString().replace(/\n/g, "\\n").replace(/\r/g, "\\r") @@ -32,6 +33,6 @@ Promise.try -> console.log "Got response" response.on "progress", (completedBytes, totalBytes, request) -> console.log "#{completedBytes / totalBytes * 100}%", completedBytes, totalBytes - response.resume() + response.pipe(devNull()) response.on "end", -> console.log "Completed response download"