proxy keeps node process alive

[zkat]

There's an issue, probably with the proxy libraries make-fetch-happen is using, where the node process is kept alive if you use m-f-h's proxying agent features. This is likely because some socket somewhere isn't closing, or the libraries are doing some keepalive thing without any timeouts.

To set up a local SOCKS proxy to test with:

1. enable SSH (in OSX, System Preferences -> Sharing -> Remote login)
2. ssh -vND 8888 "$USER@localhost"

To set up a local http(s) proxy to test with:

1. install a reverse proxy, such as Squid
2. Configure and run the proxy locally

You can use PROXY={socks,http}://localhost:8888 node to have make-fetch-happen pick it up. You should be able to hit any http or https service with m-f-h after that.

[zkat]

hint: @yyjdelete suggested trying something like JoshGlazebrook/socks@c65706d as a patch. I wonder if that's really what it would take. And why it would work.

There's an issue here that @TooTallNate's proxy projects seem to be... dead? So we may need to fork, or find something different. Perhaps it's a good idea to look into switching to https://npm.im/tunnel-agent? That's what request uses so it's definitely still maintained. I don't think it supports socks proxying though.

[pi0]

Test snippet:

const fetch = require('make-fetch-happen')
process.env.PROXY='http://icanhazip.com' // Without this line works fine
fetch('http://icanhazip.com').then(console.log).catch(console.error)

[pi0]

Tried a simple attempt to catch object leaks.

const fetch = require('make-fetch-happen')
const heapdump = require('heapdump')

const dump = () => new Promise((resolve,reject) => heapdump.writeSnapshot((err, filename) => {  
  if(err) reject(err);
  resolve();
  console.log('dump written to', filename)
}))

const test = () => fetch('http://icanhazip.com').then(console.log).catch(console.error)

test().then(dump).then(() => {
	process.env.PROXY = 'http://icanhazip.com'
	test().then(dump)
});

You can compare two heap dumps. Seems HTTPProxyAgent is keeping process open.

[TooTallNate]

There's an issue here that @TooTallNate's proxy projects seem to be... dead?

Come again? Is there a pull request I need to address?

[zkat]

@TooTallNate oh! Great! Sorry, I saw that it'd been a few years since a release and didn't wanna bother you. I might end up having a pull request for you then -- just gotta figure out why using the proxies is keeping the node process alive 👍

[TooTallNate]

Sounds great! Ya all my proxy modules have mostly been completed, hence the small amount of activity. But I'm here if you need help or find bugs.

[zkat]

@TooTallNate do you have any idea where to look for why this particular bug is happening? It's definitely only when going through one of these proxies.

[TooTallNate]

process._getActiveHandles() and/or process._getActiveRequests() might shed some light, but in general I've had a hard time making heads or tails of the results in the past.

If you can put together a simplified test case I can take a look.

[zkat]

Will do! I'll push that onto the stack :)

[yyjdelete]

@zkat @TooTallNate
JoshGlazebrook/socks@c65706d is for another issue that socks-proxy can't get response for http/ws. Tested with the example code of node-socks-proxy-agent and node 7.9

And found there is already an PR for http as TooTallNate/node-socks-proxy-agent#12.

[zkat]

I'm having a hard time coming up with a repro step isolated from npm itself :( I'll have to take a look at this much later, then, unless someone else picks it up. I've really no clue right now.

[zkat]

Heads-up: The npm-side repro case is going away as of the latest beta 'cause I worked around the dangling socket issue (by calling process.exit() directly when commands finish, whereas the CLI would usually let the process exit by itself). This is still an issue for make-fetch-happen itself, though.

[pi0]

@TooTallNate Thanx for your suggestion _getActiveHandles helped a lot. This problem should be resolved by calling unref on proxy sockets.

Calling unref on a socket will allow the program to exit if this is the only active socket in the event system. If the socket is already unrefd calling unref again will have no effect.

[TooTallNate]

@zkat @pi0 Sorry for the radio silence on this one. It's still on my radar I promise.

I'm actually having trouble reproducing the issue locally, using a modified version of the example script in the README:

const fetch = require('./').defaults({
  cacheManager: './my-cache' // path where cache will be written (and read)
})

async function main() {
  let res
  let body

  res = await fetch('https://registry.npmjs.org/make-fetch-happen')
  console.log(res.status) // 200!
  body = await res.json() // download the body as JSON
  console.log(`got ${body.name} from web`)

  res = await fetch('https://registry.npmjs.org/make-fetch-happen', {
    cache: 'no-cache' // forces a conditional request
  })
  console.log(res.status) // 304! cache validated!
  body = await res.json() // download the body as JSON
  console.log(`got ${body.name} from cache`)
}

main().catch(console.error)

When running with squid, works as expected:

$ rm -rf my-cache/ && PROXY=http://localhost:3128 node t
200
got make-fetch-happen from web
304
got make-fetch-happen from cache

Or with a SSH socks proxy setup to my iMac at home:

$ rm -rf my-cache/ && PROXY=socks://localhost:8888 node t
200
got make-fetch-happen from web
304
got make-fetch-happen from cache

In both cases the process exits as expected (this is with Node 8.0.0).

---

On a completely separate note, I notice that the agent.js file has a lot of similarities to the proxy-agent module, which is meant to be the high-level summation of my other *-proxy-agent modules.

superagent-proxy uses it to just depend on a single "proxy" module instead of "n" modules.

I think it would be really cool if m-f-h used that and we could move any necessary logic from your agent.js to proxy-agent. Let me know what you think!

[TooTallNate]

@pi0 fwiw, this line in your repro sample is not correct:

process.env.PROXY='http://icanhazip.com' // Without this line works fine

icanhazip.com is not an HTTP proxy. It is the endpoint we are trying to connect to in this example. The PROXY variable should be set up to a Squid or Socks (or equivalent) proxy server URL, like how @zkat describes in the OP.

[zkat]

@TooTallNate I was using proxy-agent before and it was quite nice but had to move away because pac-proxy-agent is massive deps-wise and it seemed like a ton of MB to pull in for a feature I don't have any immediate need to support.

As far as a repro: I, too, tried to find a minimal repro isolated even to m-f-h but failed to do so. I know the difference was between using a proxy and not, but I could only ever repro this on npm itself. It's really weird. It might have to do with number of requests, or size of requests, or something completely different that happens to be triggered by this. I've no idea.

[TooTallNate]

PAC being massive was because it was doing transpilation using regenerator for versions of Node.js that did not support Generators. pac-resolver@2 fixed this issue by dropping support for those older versions. Hopefully proxy-agent would end up being more lightweight at this point.

But I'd also be interested in looking into making all the http proxy types be opt-in as peerDependencies, and then proxy-agent would pick them up implicitly. That way m-f-h (and/or the end-user) could decide which proxy types to support.

[zkat]

oh great! I didn't like having to do this logic on m-f-h's end anyway. I'll make an issue to upgrade back to proxy-agent, then.

[jjimenez]

sorry to revive an old topic and I apologize for any misunderstanding of how things work in advance.

We recently hit this issue on our project due to being behind a proxy when doing our project install. I've tried to chase the issue down and it seems like the agent-base fix referenced here
TooTallNate/node-agent-base#18
which brings agent-base to 4.2.1 would fix the issue. It seems if this version ended up in your package_lock that would help the npm repo to bring it in. It seems like npm is trying to update pacote to take care of this issue in 6.8, which depends on make-fetch-happen and on down to agent-base, but, and again, my understanding is limited, the package-locks end up having 4.2.0 and not 4.2.1 of agent base, so it won't fix the issue.

If you think it would help, I could try to do a pull request to bump the version of agent-base that is in package_lock?

[zkat]

@jjimenez npm/cli@6b70c02 I've bumped it in release-next, so it should be included in the next npm release.

I have also published an npm canary, if you'd like to test to confirm this actually got fixed for you. You can try it using npx npmc@6.8.0-canary.2 install and see if you still get the proxy hangups!

[jjimenez]

Thanks @zkat I'll see about running a local test!