Breaking expected Node behaviour for http.get

[kennethaasan]

Related: #1588 and newrelic/node-newrelic#316

New Relic (https://github.com/newrelic/node-newrelic) is overwriting http.get and http.request like you do in https://github.com/mastermatt/nock/blob/master/lib/common.js#L102. But when http.get is calling http.request like here, a call to http.get will call http.request twice when using nock in a New Relic test.

To prove that this is not native Node behaviour (at least on Node 12):

const http = require('http');

function request() {
  console.log('http.request', arguments);
}

http.request = request;

// our request function is not called
http.get('http://google.com', () => {});

Looks like this change in https://github.com/mastermatt/nock/blob/3fbc69d74f6d23b33c2c6b144aa9fbf1d855cd18/lib/common.js#L102-L104 works:

const req = newRequest(proto, overriddenGet.bind(module), [
  input,
  options,
  callback,
])
req.end()
return req

[paulmelnikow]

I realize this has high ecosystem impact so I'd like to get this resolved and released in the next day or so.

I'm trying to write a test for the bad behavior, and unfortunately I'm not exactly clear what it is. In #1837 we discussed adding a test and one was drafted, and that one's still in #1853:

test('correct node behavior', t => {
  const scope = nock('http://example.test')
    .get('/')
    .reply()

  const reqSpy = sinon.spy(http.request)
  const origHttpReq = http.request

  http.request = reqSpy

  http.get('http://example.test', res => {
    t.equal(res.statusCode, 200)

    res.on('data', reqSpy)

    res.on('end', () => {
      t.equal(reqSpy.called, false)
      t.end()
    })
  })

  t.on('end', () => {
    scope.done()
    http.request = origHttpReq
  })
})

This seems like it's checking the implementation, rather than expressing the desirable behavior that this bug report is complaining about.

Could I get some clarification of the undesirable behavior?

Something like…

When a library like New Relic overrides http.request after Nock has already done so, and an unmocked request is made, the New Relic override of http.request is invoked twice instead of once.

It would be very helpful in resolving this.

I've tried writing a test like this, but it's not failing in a way that matches that description.

test('overridden http.get invokes original http.request only once', {only: true}, async t => {
  // Obtain the original `http.request()`, and spy on it.
  nock.restore()
  const reqSpy = sinon.spy(http, 'request')

  nock.activate()

  const server = http.createServer((request, response) => {
    response.writeHead(200)
    response.end()
  })
  await new Promise(resolve => server.listen(resolve))

  const {statusCode} = await got(`http://localhost:${server.address().port}`)
  expect(statusCode).to.equal(200)

  expect(reqSpy).to.have.been.calledOnce()

  server.close()
})

[paulmelnikow]

Most libraries do not override http.request or http.get. What I’m trying to understand is what New Relic is trying to accomplish by doing so, and how Nock’s implementation got in the way of that.

[nikaspran]

@paulmelnikow we've run into this ourselves, from my investigation it seems that the issue is with NewRelic. They're adding a proxy to monitor outgoing requests, but in a manner that's not fully compatible with Node >= 10. Specifically, we're running into these issues while running on production (so no Nock).

We've had previous issues with an older version of Nock and got 10+ but those have always been resolved upon upgrading Nock, so it doesn't seem like Nock is at fault here, at least recent versions.

[kennethaasan]

@paulmelnikow @nikaspran,

I've attempted to fix NewRelic with https://github.com/newrelic/node-newrelic/pull/318/files

That PR is fixing the problems you describe:

• Upgrade of https-proxy-agent which overwrote the native http/https APIs and caused problems with for example got 10+
• Add support for new arguments to the http/https APIs that for example got 10+ is using

But when I did that the unit tests for NewRelic started to fail because they're using Nock in their unit tests. I found out that the unit tests were failing because Nock is not doing the native node behavior. In the unit tests, on a call to http.get, NewRelic recorded the call as 2 network calls because Nock sends the request to http.request as well. #1853 fixes that and also fixes the NewRelic unit tests in that PR.

[paulmelnikow]

In the unit tests, on a call to http.get, NewRelic recorded the call as 2 network calls because Nock sends the request to http.request as well.

Since this is the thing that's being asked fo Nock, I'd like to create a test that isolates that behavior.

Is Nock's override getting loaded first, or is New Relic's?

[kennethaasan]

Sorry, I don't have time to dig into the NewRelic codebase to find out.

This is the test that's failing: https://github.com/newrelic/node-newrelic/blob/master/test/unit/instrumentation/http/http.test.js#L749-L778

[paulmelnikow]

I've opened #1868 with two new tests.

[nockbot]

🎉 This issue has been resolved in version 11.8.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀