http2 agent doesn't work with request module

[Perseid]

Hi,

I'm trying to use the http2 agent with the request module. Running

var request = require("request");
var https = require("https");
var myHttp1Agent = new https.Agent({ca:ca});
request({url:"https://foo.testing/", agent:myHttp1Agent},function(error, response, body){
  console.log("request with http1 agent");
  console.log(error,body);
});

works fine but with the http2 version

var request = require("request");
var http2 = require("http2");
var myHttp2Agent = new http2.Agent({ca:ca});
request({url:"https://foo.testing/", agent:myHttp2Agent},function(error, response, body){
  console.log("request with http2 agent");
  console.log(error,body);
});

I get an exception:

_http_client.js:137
    self.agent.addRequest(self, options);
               ^

TypeError: self.agent.addRequest is not a function
    at new ClientRequest (_http_client.js:137:16)
    at Object.exports.request (http.js:31:10)
    at Object.exports.request (https.js:181:15)
    at Request.start (/home/node/app/node_modules/request/request.js:746:30)
    at Request.end (/home/node/app/node_modules/request/request.js:1357:10)
    at end (/home/node/app/node_modules/request/request.js:574:14)
    at Immediate._onImmediate (/home/node/app/node_modules/request/request.js:588:7)
    at processImmediate [as _immediateCallback] (timers.js:383:17)

As far as I understood, http2 is supposed to be a drop in replacement for the http(s) module. So, is this a bug?

[felicienfrancois]

get the same error. If I use globalAgent (which seems to be initialized with new Agent()), it works well ...

[acarstoiu]

The fix is correct, this ticket can be closed and a new version released :+1:

[Perseid]

Thanks!

[punmechanic]

This doesn't appear to be fixed in http2@3.3.2?

My code is below. This exact code works with the baked in https module. Custom agent is required to use node-fetch with self-signed certs. Doesn't seem to work with http2.globalAgent either, unless I am missing something. node@5.10.1

import assert from 'assert'
import fetch from 'node-fetch'
import url from 'url'
import koa from 'koa'
import mount from 'koa-mount'
import http2 from 'http2'
import fs from 'fs'
import path from 'path'

const host = 'localhost'
const port = 8080

const dummyData = {
  greeting: 'hello, world!'
}

function* greetingMiddleware() {
  this.body = dummyData
  this.status = 200
}

const server = koa()
server.use(mount('/greeting', greetingMiddleware))
const http2Server = http2.createServer({
  key: fs.readFileSync(path.resolve(process.env.HOME, './https/server.key')),
  cert: fs.readFileSync(path.resolve(process.env.HOME, './https/server.crt'))
}, server.callback())
http2Server.listen(port)

const endpoint = url.format({
  protocol: 'https',
  hostname: host,
  port: port,
  pathname: '/greeting'
})

/*
 * A custom agent is required otherwise "fetch" will reject due to us
 * using a self-signed certificate. A bit silly that this is required
 * as it makes our dev environment different from test. :(
 */
const agent = new http2.Agent({
  rejectUnauthorized: false
})

setTimeout(() => {
  fetch(endpoint, { agent })
    .then(response => {
      assert.equal(response.status, 200)
      return response.json()
    })
    .then(receivedData => {
      assert.deepEqual(receivedData, dummyData)
    })
    .catch(error => console.error(error))
    .catch(() => http2Server.close())
    .then(() => http2Server.close())
})

The proposed fix also does not seem to resolve the issue.

[felicienfrancois]

@danpantry The pull request is not yet merged. This project seems no longer maintained by its owners (is it?). In the meantime you can switch to my fork : https://github.com/felicienfrancois/node-http2 which includes the fix (among others) in your package.json:

  "dependencies": {
    "http2": "felicienfrancois/node-http2",
  }

[punmechanic]

@felicienfrancois Cheers, thanks for the information.

[nwgh]

Not abandoned, but just not a whole lot of time to deal with this in the face of other commitments.

As noted in #184, the "fix" here is actually most likely wrong, and a better fix should be written.

[felicienfrancois]

@nwgh ok thank to hear you. I would be glad to makes another PR if you could tell me what should be the behavior.

This issue is not a compatibility issue. it is a breaking one. It prevents usage of agent options of http2.request Here the actual behavior:

• http2.request accept options.agent attribute and expect an http2.Agent API otherwise it break with request is not defined
• http2.request delegates the request to either options.agent or the internal http2.Agent(), and forward the whole options, including the agent options
• http2.Agent.prototype.request accept options.agent attribute and expect an https.Agent API otherwise it break with addRequest is not a function

The fix #184 was to ignore agent option in http2.Agent.prototype.request because I think this is a non-sense to specify an https.agent to an http2.agent at request time. A custom https.Agent could still be provided to an http2.Agent at creation time:

var agent = new http2.Agent({
    agent: new https.Agent(someSettings)
})

If you still want to keep to possibility to pass an https Agent at request time, we should either

• filter the agent option when passing options from http2.request to Agent.prototype.request
• rename the options to avoid conflict (example: httpsAgent and agent or httpsAgent and http2Agent) and allow to pass both options to http2.request

[nwgh]

Yes, the behavior here is definitely broken (I marked this "compatibility" as it breaks the ability to be used with the request module as a drop-in replacement for https).

I think the right course here is to filter the agent option when calling Agent.prototype.request from http2.requestTLS (and friends) - it's obviously conflating the two agent types. What I think should happen is, instead of just doing

return (options.agent || exports.globalAgent).request(options, callback);

we should check the type of options.agent - if it's an http2.Agent, remove it from options and return agent.request(options, callback);, otherwise we can just go with exports.globalAgent.request(options, callback);

@felicienfrancois are you willing to write up the PR for this?

[richardkazuomiller]

I'm getting the same error self.agent.addRequest is not a function using node-fetch and http2@3.3.6. @danpantry did you ever get this to work?

[punmechanic]

@richardkazuomiller I followed the 'fix' posted by @felicienfrancois.

[felicienfrancois]

@richardkazuomiller are you sure you're using http2@3.3.6 ? Because self.agent.addRequest is no longer part of http2 code in 3.3.6.

You should check your npm cache and reload dependencies

[Robbilie]

@felicienfrancois using request and setting http2.Agent with agentClass you get the error @richardkazuomiller mentions when making requests…

> TypeError: self.agent.addRequest is not a function
    at new ClientRequest (_http_client.js:159:16)
    at Object.request (http.js:26:10)
    at Object.request (https.js:199:15)
    at Request.start (/home/rschuh/projects/request-http2/node_modules/request/request.js:744:32)
    at Request.end (/home/rschuh/projects/request-http2/node_modules/request/request.js:1435:10)
    at end (/home/rschuh/projects/request-http2/node_modules/request/request.js:566:14)
    at Immediate.<anonymous> (/home/rschuh/projects/request-http2/node_modules/request/request.js:580:7)
    at runCallback (timers.js:651:20)
    at tryOnImmediate (timers.js:624:5)
    at processImmediate [as _immediateCallback] (timers.js:596:5) undefined

[richardkazuomiller]

FWIW I gave up and I'm using spdy now

[Robbilie]

which doesnt work for me and throws a giant amount of " Error Got RST REFUSED_STREAM " when requesting a google kubernetes cluster, also no significant RPS increase over normal http