Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Work in Progress: Request re-write for new streams. #660

Closed
wants to merge 1 commit into from

5 participants

@mikeal
Owner

This is nowhere near finished and most of the tests still don't pass.

This is full re-write of request from the ground up. It is built on top of new style streams (streams2) and there is a concerted effort to do more of the work in dependencies.

Deprecations

  • piping to a destination w/ a callback parameter forces buffering.

request(url, function (e, resp) { }).pipe(fs.createWriteStream(file) will now have the side effect of also buffering the entire response in to memory. Because new style streams can be piped well after nextTick() we don't have a good way to know if we should or should not buffer the response other than the callback parameter. we still will not ever buffer the response when a callback is not given.

  • JSON.stringify(request(url)) probably breaks.

Deprecating the old hacky getSafe() function will dramatically improve performance and, for the most part, stringifying options with sockets attached is a bad idea.

  • all signing, cookies, and other non-default options will require the user explicitly install their dependency.

The dep list is growing exponentially and because of the way cookies and signing work in particular it isn't possible for request to support these features without adding code to request itself. Rather than continue to let the dep list grow forever in to the future, and with the understanding that one day we may even want a feature that requires a compiled dependency, I'm going to move to a model where we check for the dep when you use the feature and throw if you didn't install it.

This will not be the case for default options which will still have their deps included.

Features

  • NEW STYLE STREAMS!

  • gzip support, enabled by default.

We now default to including the Accept-Encoding header for gzip and transparently decompressing the stream in the underlying HTTP Duplex stream.

  • less memory copies when using binary response buffering.

Basically we just moved to @rvagg's bl module which we pipe ourself to in order to buffer the response. bl gives us a compatible buffer API but actually stores all the chunks in an array so we save on the big memcopy we used to do.

The code is also much cleaner, which is always the case when you re-write a module that hasn't had a full re-write since node.js had promises built in :)

@nylen
Owner

Cool!

Can we use this opportunity to rework how JSON requests/responses are handled? Right now the .json() method both sends the request as JSON, and accepts the response as JSON.

This has caused problems with me when interacting with the MediaWiki API - I'd like request to decode JSON requests for me, but the API can't accept JSON post bodies.

Some thoughts:

  • It should be possible to control these two behaviors separately. Perhaps with a second parameter?

  • Should the default be to decode responses with Content-Type: application/json, unless this is overridden somehow?

Let me know what you think and I will work something up on the 3.0 branch.

@mikeal
Owner

@nylen doesn't it work if you just do .json(true).form({k:v})?

@mikeal mikeal referenced this pull request
Closed

Optional GZIP compression #234

@jokeyrhyme

Ah, this explains why I was getting std:bad_alloc crashes. I was using a callback with .pipe() to download a very large file. I've removed the callback now, and the crash seems to be gone. I'm not using this fork (yet), but I thought I'd mention this here just so that others might find an explanation.

Interestingly, memory usage still seems very high, but it seems to flush regularly enough to keep from hitting 100%.

@FredKSchott
Collaborator

re "explicitly installing dependencies": A pattern that I've seen and enjoyed working with involves exposing options that allow the consumer to pass in the instance of a dependency that they'd like the module to use. On top of being explicit, it would also give the consumer a chance to setup/customize the dependency module before passing it to request. However if request is used in a lot of different places across an application, this pattern could get in the way.

It seems like this rewrite has stalled a bit (totally understandable, everyone's busy :). Are you looking for any help? I may have some time to dedicate in a few weeks and would love to help out if I can.

@mikeal
Owner

A pattern that I've seen and enjoyed working with involves exposing options that allow the consumer to pass in the instance of a dependency that they'd like the module to use.

See #1005

@mikeal mikeal closed this
@nylen
Owner

Did you mean to close this?

@mikeal
Owner

Nah, I meant to close the other one, I'm going to have to redo that whole branch so I don't want to leave the PR open.

@mikeal
Owner

I mean, I meant to close this one :)

@nylen
Owner

OK. Just a sanity check and/or documentation :)

@wprater

@mikeal this was not merged in yet, correct? Im using 2.40.1 and downloading a lot of image files and saving them to disk. It seems to be buffering the responses as my memory jumps up each time I fetch images. I've tried to not provide a callback, but it seems the buffering is still happening. I need to use the callback to check if there were errors in the request and pass that callback to my async queue.

Is there a better way to go about this, or to force a flash of the buffer?

some pseudo code.

    var photoStream = fs.createWriteStream(photoPath);
    request({
      uri: config.blobEndpoint,
      qs: { key: blobKey },
      timeout: 5000
    }, function (err) { if (err) callback(err); })
    .pipe(photoStream);
    photoStream.on('finish', function () {
      callback(null, blobKey);
    });
    photoStream.on('error', function () {
      console.error('error writing the file.');
      callback(new Error('error writing the file.'));
    });
@mikeal
Owner
@jokeyrhyme

@mikeal haha, I just literally came to the same conclusion on one of my projects at work. Glad to see I'm not the only one this happens to. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 13, 2013
  1. @mikeal
This page is out of date. Refresh to see the latest.
Showing with 228 additions and 75 deletions.
  1. +0 −34 lib/getSafe.js
  2. +180 −0 lib/index.js
  3. +9 −7 package.json
  4. +1 −1  tests/test-agentOptions.js
  5. +1 −1  tests/test-basic-auth.js
  6. +8 −3 tests/test-body.js
  7. +1 −1  tests/test-defaults.js
  8. +1 −1  tests/test-digest-auth.js
  9. +1 −1  tests/test-emptyBody.js
  10. +1 −1  tests/test-errors.js
  11. +1 −1  tests/test-follow-all-303.js
  12. +1 −1  tests/test-follow-all.js
  13. +1 −1  tests/test-form.js
  14. +1 −1  tests/test-hawk.js
  15. +1 −1  tests/test-headers.js
  16. +1 −1  tests/test-http-signature.js
  17. +1 −1  tests/test-httpModule.js
  18. +1 −1  tests/test-https-strict.js
  19. +1 −1  tests/test-https.js
  20. +1 −1  tests/test-isUrl.js
  21. +1 −1  tests/test-localAddress.js
  22. +1 −1  tests/test-oauth.js
  23. +1 −1  tests/test-onelineproxy.js
  24. +1 −1  tests/test-params.js
  25. +1 −1  tests/test-piped-redirect.js
  26. +1 −1  tests/test-pipes.js
  27. +1 −1  tests/test-pool.js
  28. +1 −1  tests/test-protocol-changing-redirect.js
  29. +1 −1  tests/test-proxy.js
  30. +1 −1  tests/test-qs.js
  31. +1 −1  tests/test-redirect.js
  32. +1 −1  tests/test-s3.js
  33. +1 −1  tests/test-timeout.js
  34. +1 −1  tests/test-toJSON.js
  35. +1 −1  tests/test-tunnel.js
View
34 lib/getSafe.js
@@ -1,34 +0,0 @@
-// Safe toJSON
-module.exports =
-function getSafe (self, uuid) {
- if (typeof self === 'object' || typeof self === 'function') var safe = {}
- if (Array.isArray(self)) var safe = []
-
- var recurse = []
-
- Object.defineProperty(self, uuid, {})
-
- var attrs = Object.keys(self).filter(function (i) {
- if (i === uuid) return false
- if ( (typeof self[i] !== 'object' && typeof self[i] !== 'function') || self[i] === null) return true
- return !(Object.getOwnPropertyDescriptor(self[i], uuid))
- })
-
-
- for (var i=0;i<attrs.length;i++) {
- if ( (typeof self[attrs[i]] !== 'object' && typeof self[attrs[i]] !== 'function') ||
- self[attrs[i]] === null
- ) {
- safe[attrs[i]] = self[attrs[i]]
- } else {
- recurse.push(attrs[i])
- Object.defineProperty(self[attrs[i]], uuid, {})
- }
- }
-
- for (var i=0;i<recurse.length;i++) {
- safe[recurse[i]] = getSafe(self[recurse[i]], uuid)
- }
-
- return safe
-}
View
180 lib/index.js
@@ -0,0 +1,180 @@
+var util = require('util')
+ , url = require('url')
+ , qs = require('querystring')
+
+ , HTTPDuplex = require('http-duplex-client')
+ , HTTPGzipDuplex = require('http-duplex-gzip-client')
+ , BL = require('bl')
+
+ , defaults = require('defaults')
+ , safeStringify = require('json-stringify-safe')
+ , once = require('once')
+ , caseless = require('caseless')
+
+ , debug = require('./debug')
+
+ , noop = function () {}
+
+ , _http =
+ { 'http:': require('http')
+ , 'https:': require('https')
+ }
+
+ , _defaults =
+ { gzip: true
+ , bl: undefined
+ , headers: {}
+ , method: 'GET'
+
+ , localAddress: undefined
+ , socketPath: undefined
+ , agent: undefined
+
+ , encoding: 'utf8'
+ }
+ ;
+
+function toJSON (val) {
+ try {
+ return JSON.stringify(val)
+ } catch (e) {
+ return safeStringify(val)
+ }
+}
+
+function makeRequest () {
+ var opts =
+ { method: this.method
+ , path: this._uri.path
+ , port: this._uri.port
+ , hostname: this._uri.hostname
+ , host: this._uri.host
+ , localAddress: this.localAddress
+ , socketPath: this.socketPath
+ , headers: this._headers.dict
+ , agent: this.agent
+ }
+
+ this.http = _http[this._uri.protocol]
+ if (!this.http) this.emit('error', new Error('Unknown protocol scheme "'+this._uri.protocol+'"'))
+
+ if (this.gzip) HTTPGzipDuplex.prototype.makeRequest.call(this, opts)
+ else HTTPDuplex.prototype.makeRequest.call(this, opts)
+}
+
+function parseOptions (options) {
+ options = defaults(options, _defaults)
+ if (options.url) options.uri = url.parse(options.url)
+ return options
+}
+
+util.inherits(Request, HTTPGzipDuplex)
+function Request (options) {
+ this._headers = undefined
+ this._json = false
+ this.body = undefined
+
+ options = parseOptions(options)
+
+ this.headers(options.headers)
+ delete options.headers
+
+ for (var i in options) {
+ if (i[0] === '_' || !this[i]) this[i] = options[i]
+ else this[i](options[i])
+ }
+
+ if (this.callback) this.callback = once(this.callback)
+
+ var self = this
+ this.on('response', function (resp) { self.response = resp })
+ this.on('body', function () {
+ if (self.encoding !== null && self.encoding !== 'binary') {
+ self.response.body = self.response.body.toString(self.encoding)
+ }
+ if (self._json) {
+ try {
+ self.response.body = JSON.parse(self.response.body)
+ } catch (e) {}
+ }
+ })
+
+ process.nextTick(this.start.bind(this))
+ HTTPGzipDuplex.call(this, {}, options.stream)
+ this.makeRequest = makeRequest
+}
+Request.prototype.start = function () {
+ var self = this
+ if (this.callback) {
+ this.bl = new BL()
+ this.pipe(this.bl)
+
+ this.on('end', function () {
+ self.response.body = self.bl
+ this.emit('body')
+ self.callback(null, self.response, self.response.body)
+ })
+ this.on('error', function (err) {
+ self.callback(err)
+ })
+ }
+
+ this.makeRequest()
+ if (this.method === 'GET' || this.method === 'HEAD') {
+ this.end()
+ }
+ if (this.body) {
+ if (!Buffer.isBuffer(this.body)) this.body = new Buffer(this.body)
+ this.setHeader('content-length', this.body.length)
+ this.write(this.body)
+ this.end()
+ }
+}
+Request.prototype.makeRequest = noop
+Request.prototype.json = function (val) {
+ if (val !== false) {
+ this._json = true
+ this.setHeader('accept', 'application/json')
+ }
+ if (typeof val !== 'boolean') {
+ this.setHeader('content-type', 'application/json')
+ this.body = new Buffer(toJSON(val))
+ }
+}
+Request.prototype.headers = function (headers) {
+ this._headers = caseless(headers)
+}
+Request.prototype.setHeader = function (name, value, clobber) {
+ return this._headers.set(name, value, clobber)
+}
+Request.prototype.hasHeader = function (name) {
+ return this._headers.has(name)
+}
+Request.prototype.setHeaders = function (headers, clobbler) {
+ return this._headers.set(name, clobber)
+}
+Request.prototype.url = function (u) {
+ if (typeof u === 'string') u = url.parse(u)
+ this._uri = u
+}
+Request.prototype.uri = Request.prototype.url
+
+function request (uri, options, callback) {
+ if (typeof uri === 'undefined') throw new Error('undefined is not a valid uri or options object.')
+ if ((typeof options === 'function') && !callback) callback = options
+ if (options && typeof options === 'object') {
+ options.uri = uri
+ } else if (typeof uri === 'string') {
+ options = {uri:uri}
+ } else {
+ options = uri
+ }
+
+ options = defaults({}, options)
+
+ if (callback) options.callback = callback
+ var r = new Request(options)
+ return r
+}
+
+module.exports = request
View
16 package.json
@@ -7,7 +7,7 @@
"util",
"utility"
],
- "version": "2.27.1",
+ "version": "3.0.0",
"author": "Mikeal Rogers <mikeal.rogers@gmail.com>",
"repository": {
"type": "git",
@@ -19,20 +19,22 @@
"engines": [
"node >= 0.8.0"
],
- "main": "index.js",
+ "main": "lib/index.js",
"dependencies": {
- "qs": "~0.6.0",
"json-stringify-safe": "~5.0.0",
- "forever-agent": "~0.5.0",
- "tunnel-agent": "~0.3.0",
"http-signature": "~0.10.0",
"hawk": "~1.0.0",
"aws-sign": "~0.3.0",
"oauth-sign": "~0.3.0",
"cookie-jar": "~0.3.0",
- "node-uuid": "~1.4.0",
"mime": "~1.2.9",
- "form-data": "~0.1.0"
+ "form-data": "~0.1.0",
+ "http-duplex-client": "~0.4.0",
+ "http-duplex-gzip-client": "~0.1.0",
+ "bl": "~0.4.1",
+ "once": "~1.2.0",
+ "defaults": "~1.0.0",
+ "caseless": "~0.1.0"
},
"scripts": {
"test": "node tests/run.js"
View
2  tests/test-agentOptions.js
@@ -1,4 +1,4 @@
-var request = require('../index')
+var request = require('../lib/index.js')
, http = require('http')
, server = require('./server')
, assert = require('assert')
View
2  tests/test-basic-auth.js
@@ -1,6 +1,6 @@
var assert = require('assert')
, http = require('http')
- , request = require('../index')
+ , request = require('../lib/index.js')
;
var numBasicRequests = 0;
View
11 tests/test-body.js
@@ -2,7 +2,7 @@ var server = require('./server')
, events = require('events')
, stream = require('stream')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
;
var s = server.createServer();
@@ -38,7 +38,7 @@ var tests =
, testGetUTF8:
{ resp: server.createGetResponse(new Buffer([0xEF, 0xBB, 0xBF, 226, 152, 131]))
, encoding: "utf8"
- , expectBody: ""
+ , expectBody: new Buffer([0xEF, 0xBB, 0xBF, 226, 152, 131]).toString('utf8')
}
, testGetJSON :
{ resp : server.createGetResponse('{"test":true}', 'application/json')
@@ -107,9 +107,14 @@ s.listen(s.port, function () {
request(test, function (err, resp, body) {
if (err) throw err
if (test.expectBody) {
- assert.deepEqual(test.expectBody, body)
+ if (Buffer.isBuffer(test.expectBody)) {
+ assert.equal(test.expectBody.toString('base64'), body.toString('base64'))
+ } else {
+ assert.deepEqual(test.expectBody, body)
+ }
}
counter = counter - 1;
+ console.log(test.uri, counter)
if (counter === 0) {
console.log(Object.keys(tests).length+" tests passed.")
s.close()
View
2  tests/test-defaults.js
@@ -1,6 +1,6 @@
var server = require('./server')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
;
var s = server.createServer();
View
2  tests/test-digest-auth.js
@@ -1,6 +1,6 @@
var assert = require('assert')
, http = require('http')
- , request = require('../index')
+ , request = require('../lib/index.js')
;
// Test digest auth
View
2  tests/test-emptyBody.js
@@ -1,4 +1,4 @@
-var request = require('../index')
+var request = require('../lib/index.js')
, http = require('http')
, assert = require('assert')
;
View
2  tests/test-errors.js
@@ -1,7 +1,7 @@
var server = require('./server')
, events = require('events')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
;
var local = 'http://localhost:8888/asdf'
View
2  tests/test-follow-all-303.js
@@ -1,4 +1,4 @@
-var request = require('../index');
+var request = require('../lib/index.js');
var http = require('http');
var requests = 0;
var assert = require('assert');
View
2  tests/test-follow-all.js
@@ -1,4 +1,4 @@
-var request = require('../index');
+var request = require('../lib/index.js');
var http = require('http');
var requests = 0;
var assert = require('assert');
View
2  tests/test-form.js
@@ -2,7 +2,7 @@ var assert = require('assert')
var http = require('http');
var path = require('path');
var mime = require('mime');
-var request = require('../index');
+var request = require('../lib/index.js');
var fs = require('fs');
var remoteFile = 'http://nodejs.org/images/logo.png';
View
2  tests/test-hawk.js
@@ -1,5 +1,5 @@
var createServer = require('http').createServer
- , request = require('../index')
+ , request = require('../lib/index.js')
, hawk = require('hawk')
, assert = require('assert')
;
View
2  tests/test-headers.js
@@ -1,6 +1,6 @@
var server = require('./server')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
, Cookie = require('cookie-jar')
, Jar = Cookie.Jar
, s = server.createServer()
View
2  tests/test-http-signature.js
@@ -1,5 +1,5 @@
var createServer = require('http').createServer
- , request = require('../index')
+ , request = require('../lib/index.js')
, httpSignature = require('http-signature')
, assert = require('assert')
;
View
2  tests/test-httpModule.js
@@ -2,7 +2,7 @@ var http = require('http')
, https = require('https')
, server = require('./server')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
var faux_requests_made = {'http':0, 'https':0}
View
2  tests/test-https-strict.js
@@ -3,7 +3,7 @@
var server = require('./server')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
, fs = require('fs')
, path = require('path')
, opts = { key: path.resolve(__dirname, 'ssl/ca/server.key')
View
2  tests/test-https.js
@@ -1,6 +1,6 @@
var server = require('./server')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
var s = server.createSSLServer();
View
2  tests/test-isUrl.js
@@ -1,5 +1,5 @@
var assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
, http = require('http')
;
View
2  tests/test-localAddress.js
@@ -1,4 +1,4 @@
-var request = require('../index')
+var request = require('../lib/index.js')
, assert = require('assert')
;
View
2  tests/test-oauth.js
@@ -1,7 +1,7 @@
var hmacsign = require('oauth-sign').hmacsign
, assert = require('assert')
, qs = require('querystring')
- , request = require('../index')
+ , request = require('../lib/index.js')
;
function getsignature (r) {
View
2  tests/test-onelineproxy.js
@@ -1,6 +1,6 @@
var http = require('http')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
;
var server = http.createServer(function (req, resp) {
View
2  tests/test-params.js
@@ -1,6 +1,6 @@
var server = require('./server')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
;
var s = server.createServer();
View
2  tests/test-piped-redirect.js
@@ -1,6 +1,6 @@
var http = require('http')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
;
var portOne = 8968
View
2  tests/test-pipes.js
@@ -3,7 +3,7 @@ var server = require('./server')
, stream = require('stream')
, assert = require('assert')
, fs = require('fs')
- , request = require('../index')
+ , request = require('../lib/index.js')
, path = require('path')
, util = require('util')
;
View
2  tests/test-pool.js
@@ -1,4 +1,4 @@
-var request = require('../index')
+var request = require('../lib/index.js')
, http = require('http')
, assert = require('assert')
;
View
2  tests/test-protocol-changing-redirect.js
@@ -1,6 +1,6 @@
var server = require('./server')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
var s = server.createServer()
View
2  tests/test-proxy.js
@@ -3,7 +3,7 @@ var server = require('./server')
, stream = require('stream')
, assert = require('assert')
, fs = require('fs')
- , request = require('../index')
+ , request = require('../lib/index.js')
, path = require('path')
, util = require('util')
;
View
2  tests/test-qs.js
@@ -1,4 +1,4 @@
-var request = request = require('../index')
+var request = request = require('../lib/index.js')
, assert = require('assert')
;
View
2  tests/test-redirect.js
@@ -1,6 +1,6 @@
var server = require('./server')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
, Cookie = require('cookie-jar')
, Jar = Cookie.Jar
;
View
2  tests/test-s3.js
@@ -1,4 +1,4 @@
-var request = require('../index')
+var request = require('../lib/index.js')
var r = request.get('https://log.curlybracecast.com.s3.amazonaws.com/',
{ aws:
View
2  tests/test-timeout.js
@@ -2,7 +2,7 @@ var server = require('./server')
, events = require('events')
, stream = require('stream')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
;
var s = server.createServer();
View
2  tests/test-toJSON.js
@@ -1,4 +1,4 @@
-var request = require('../index')
+var request = require('../lib/index.js')
, http = require('http')
, assert = require('assert')
;
View
2  tests/test-tunnel.js
@@ -6,7 +6,7 @@
var server = require('./server')
, assert = require('assert')
- , request = require('../index')
+ , request = require('../lib/index.js')
, fs = require('fs')
, path = require('path')
, caFile = path.resolve(__dirname, 'ssl/npm-ca.crt')
Something went wrong with that request. Please try again.