Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ test.js
components
test/node/fixtures/tmp.json
.idea
superagent.js
superagent.js
package-lock.json
7 changes: 7 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,10 @@ matrix:
include:
- node_js: "9"
env: BROWSER=1
- node_js: "9"
env: EXPOSE_HTTP2=1
- node_js: "8"
env: EXPOSE_HTTP2=1

before_install:
- "test -z $(echo $EXPOSE_HTTP2) || npm install --only=dev https://github.com/sogaani/express.git#initial-support-http2"
10 changes: 9 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,15 @@ test:
@if [ "x$(BROWSER)" = "x" ]; then make test-node; else make test-browser; fi

test-node:
@NODE_ENV=test NODE_TLS_REJECT_UNAUTHORIZED=0 ./node_modules/.bin/mocha \
@NODE_ENV=test NODE_TLS_REJECT_UNAUTHORIZED=0 node ./node_modules/.bin/mocha \
--require should \
--trace-warnings \
--reporter $(REPORTER) \
--timeout 5000 \
$(NODETESTS)

test-node-http2:
@NODE_ENV=test EXPOSE_HTTP2=1 NODE_TLS_REJECT_UNAUTHORIZED=0 node ./node_modules/.bin/mocha \
--require should \
--trace-warnings \
--reporter $(REPORTER) \
Expand Down
186 changes: 186 additions & 0 deletions lib/node/http2wrapper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
'use strict';

const http2 = require('http2');
const Stream = require('stream');
const util = require('util');
const net = require('net');
const tls = require('tls');
const parse = require('url').parse;

const {
HTTP2_HEADER_PATH,
HTTP2_HEADER_STATUS,
HTTP2_HEADER_METHOD,
HTTP2_HEADER_AUTHORITY,
HTTP2_HEADER_HOST,
HTTP2_HEADER_SET_COOKIE,
NGHTTP2_CANCEL,
} = http2.constants;


function setProtocol(protocol) {
return {
request: function (options) {
return new Request(protocol, options);
}
}
}

function Request(protocol, options) {
Stream.call(this);
const defaultPort = protocol === 'https' ? 443 : 80;
const defaultHost = 'localhost'
const port = options.port || defaultPort;
const host = options.host || defaultHost;

delete options.port
delete options.host

this.method = options.method;
this.path = options.path;
this.protocol = protocol;
this.host = host;

delete options.method
delete options.path

const sessionOptions = Object.assign({}, options);
if (options.socketPath) {
sessionOptions.socketPath = options.socketPath;
sessionOptions.createConnection = this.createUnixConnection.bind(this);
}

this._headers = {};

const session = http2.connect(`${protocol}://${host}:${port}`, sessionOptions);
this.setHeader('host', `${host}:${port}`)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary here? In normal implementation we allow options.host to be different than host header, because it's useful to do request.get('http://[ip address]').host('example.com') to force request to connect to a particular IP address.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think current implementation supports request.get('http://[ip address]').host('example.com'). I will check later 👍 .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not send header here and we can change Host header by set method. I add test for this.


session.on('error', (err) => this.emit('error', err));

this.session = session;
}

/**
* Inherit from `Stream` (which inherits from `EventEmitter`).
*/
util.inherits(Request, Stream);

Request.prototype.createUnixConnection = function (authority, options) {
switch (this.protocol) {
case 'http':
return net.connect(options.socketPath);
case 'https':
options.ALPNProtocols = ['h2'];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK browsers also send http/1 here. Do we support a fallback?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current implement does not support fall back. I can implement fallback, I plan to take fallback after this PR. Or Should I integrate with this PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. If there's no fallback, then that's OK.

options.servername = this.host;
options.allowHalfOpen = true;
return tls.connect(options.socketPath, options);
default:
throw new Error('Unsupported protocol', this.protocol);
}
}

Request.prototype.setNoDelay = function (bool) {
this.session.socket.setNoDelay(bool);
}

Request.prototype.getFrame = function () {
if (this.frame) {
return this.frame;
}

const method = {
[HTTP2_HEADER_PATH]: this.path,
[HTTP2_HEADER_METHOD]: this.method,
}

let headers = this.mapToHttp2Header(this._headers);

headers = Object.assign(headers, method);

const frame = this.session.request(headers);
frame.once('response', (headers, flags) => {
headers = this.mapToHttpHeader(headers);
frame.headers = headers;
frame.status = frame.statusCode = headers[HTTP2_HEADER_STATUS];
this.emit('response', frame);
});

this._headerSent = true;

frame.once('drain', () => this.emit('drain'));
frame.on('error', (err) => this.emit('error', err));
frame.on('close', () => this.session.close());

this.frame = frame;
return frame;
}

Request.prototype.mapToHttpHeader = function (headers) {
const keys = Object.keys(headers);
const http2Headers = {};
for (var i = 0; i < keys.length; i++) {
let key = keys[i];
let value = headers[key];
key = key.toLowerCase();
switch (key) {
case HTTP2_HEADER_SET_COOKIE:
value = Array.isArray(value) ? value : [value];
break;
default:
break;
}
http2Headers[key] = value;
}
return http2Headers;
}

Request.prototype.mapToHttp2Header = function (headers) {
const keys = Object.keys(headers);
const http2Headers = {};
for (var i = 0; i < keys.length; i++) {
let key = keys[i];
let value = headers[key];
key = key.toLowerCase();
switch (key) {
case HTTP2_HEADER_HOST:
key = HTTP2_HEADER_AUTHORITY;
value = value.startsWith('http') ? parse(value).host : value;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is such conditional parsing needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is add for compatibility with Http/1. HTTP2_HEADER_AUTHORITY(alternative Host in http/2) only support domain string, otherwise it raise error. However, I met Host header with schema(like Host: http://example.com) in some express tests.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it would be better to improve the check, otherwise host like httparchive.org would be misparsed :)

/^https?:\/\//.test(value)

break;
default:
break;
}
http2Headers[key] = value;
}
return http2Headers;
}

Request.prototype.setHeader = function (name, value) {
this._headers[name.toLowerCase()] = value;
}

Request.prototype.getHeader = function (name) {
return this._headers[name.toLowerCase()];
}

Request.prototype.write = function (data, encoding) {
const frame = this.getFrame();
return frame.write(data, encoding);
};

Request.prototype.pipe = function (stream, options) {
const frame = this.getFrame();
return frame.pipe(stream, options);
}

Request.prototype.end = function (data) {
const frame = this.getFrame();
frame.end(data);
}

Request.prototype.abort = function (data) {
const frame = this.getFrame();
frame.close(NGHTTP2_CANCEL);
this.session.close();
}

exports.setProtocol = setProtocol;
23 changes: 21 additions & 2 deletions lib/node/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ const Stream = require('stream');
const utils = require('../utils');
const unzip = require('./unzip').unzip;
const mime = require('mime');
const https = require('https');
const http = require('http');
let https = require('https');
let http = require('http');
const fs = require('fs');
const qs = require('qs');
const zlib = require('zlib');
Expand Down Expand Up @@ -82,6 +82,22 @@ exports.protocols = {
'https:': https,
};

/**
* Enable http2 if supported
*/

if(process.env.EXPOSE_HTTP2){
try {
const http2 = require('./http2wrapper');
exports.protocols = {
'http:': http2.setProtocol('http'),
'https:': http2.setProtocol('https'),
};
console.warn('superagent: Enable experimental feature http2');
} catch (_) {
}
}

/**
* Default serialization map.
*
Expand Down Expand Up @@ -787,6 +803,9 @@ Request.prototype._emitResponse = function(body, files) {
if (undefined !== body) {
response.body = body;
}
if (undefined === response.text && undefined !== body){
response.text = this._resBuffered ? null : body.toString();
}
response.files = files;
if (this._endCalled) {
response.pipe = function() {
Expand Down
4 changes: 2 additions & 2 deletions lib/node/unzip.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ exports.unzip = (req, res) => {
const _on = res.on;
res.on = function(type, fn) {
if ('data' == type || 'end' == type) {
stream.on(type, fn);
stream.on(type, fn.bind(res));
} else if ('error' == type) {
stream.on(type, fn);
stream.on(type, fn.bind(res));
_on.call(res, type, fn);
} else {
_on.call(res, type, fn);
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
"description": "elegant & feature rich browser / node HTTP with a fluent API",
"scripts": {
"prepare": "make all",
"test": "make test"
"test": "make test",
"test-http2": "make test-node-http2"
},
"keywords": [
"http",
Expand Down
3 changes: 2 additions & 1 deletion test/json.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const setup = require('./support/setup');
const uri = setup.uri;
const doesntWorkInBrowserYet = setup.NODE;
const doesntWorkInHttp2 = !process.env.EXPOSE_HTTP2;

const assert = require('assert');
const request = require('../');
Expand Down Expand Up @@ -78,7 +79,7 @@ describe('req.send(Object) as "json"', function(){
});
});

if (doesntWorkInBrowserYet) it('should work with GET', done => {
if (doesntWorkInBrowserYet && doesntWorkInHttp2) it('should work with GET', done => {
request
.get(`${uri}/echo`)
.send({ tobi: 'ferret' })
Expand Down
7 changes: 6 additions & 1 deletion test/node/agency.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ const assert = require("assert");
const should = require("should");
const cookieParser = require("cookie-parser");
const session = require("express-session");
let http = require('http');
if(process.env.EXPOSE_HTTP2){
http = require('http2');
}

app.use(cookieParser());
app.use(
Expand Down Expand Up @@ -60,7 +64,8 @@ app.get("/simple", (req, res) => {
let base = "http://localhost";
let server;
before(function listen(done) {
server = app.listen(0, function listening() {
server = http.createServer(app);
server = server.listen(0, function listening() {
base += `:${server.address().port}`;
done();
});
Expand Down
3 changes: 2 additions & 1 deletion test/node/basic.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const fs = require("fs");
const EventEmitter = require("events").EventEmitter;
const StringDecoder = require("string_decoder").StringDecoder;
const url = require("url");
const doesntWorkInHttp2 = !process.env.EXPOSE_HTTP2;

describe("[node] request", () => {
describe("with an url", () => {
Expand Down Expand Up @@ -299,7 +300,7 @@ describe("[node] request", () => {
});
});

it("should send body with .get().send()", next => {
if(doesntWorkInHttp2) it("should send body with .get().send()", next => {
request
.get(`${base}/echo`)
.set("Content-Type", "text/plain")
Expand Down
Loading