Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: allow passing a http.Agent to ipfs-http-client in node #3474

Merged
merged 13 commits into from Jan 13, 2021

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Jan 12, 2021

Right now no http.Agent is used for requests made using the http client in node, which means each request opens a new connection which can end up hitting process resource limits which means connections get dropped.

The change here sets a default http.Agent with a keepAlive: true and maxSockets of 6 which is consistent with browsers and native apps.

The user can override the agent passed to the ipfs-http-client constructor to restore the previous functionality:

const http = require('http')
const createClient = require('ipfs-http-client')

const client = createClient({
  url: 'http://127.0.0.1:5002',
  agent: new http.Agent({
    keepAlive: false,
    maxSockets: Infinity
  })
})

Refs: #3464

Right now no `http.Agent` is used for requests made using the http
client in node, which means each request opens a new connection which
can end up hitting process resource limits which means connections get
dropped.

The change here sets a default `http.Agent` with a `keepAlive: true` and
`maxSockets` of 6 which is consistent with [browsers](https://tools.ietf.org/html/rfc2616#section-8.1.4)
and [native apps](https://developer.apple.com/documentation/foundation/nsurlsessionconfiguration/1407597-httpmaximumconnectionsperhost?language=objc).

The user can override the agent passed to the ipfs-http-client constructor
to restore the previous functionality:

```
const http = require('http')
const createClient = require('ipfs-http-client')

const client = createClient({
  url: 'http://127.0.0.1:5002',
  agent: new http.Agent({
    keepAlive: false,
    maxSockets: Infinity
  })
})
```

Refs: #3464
Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Looks good to me. I would however suggest making keepAlive and makSockets (or better yet maxConnections) as options directly instead of tying it all to the Agent, so it is more immune to changes in the implementation (e.g. switching to library that does not allow providing Agent).

@achingbrain
Copy link
Member Author

I would however suggest making keepAlive and makSockets (or better yet maxConnections) as options directly instead of tying it all to the Agent

It's all a tradeoff. If we specify all agent options as part of our API, we need to ensure compatibility with whatever options are available in a given version of node or otherwise handle the incompatibility. If we start renaming options, we break compatibility with node and users have to learn our API instead of using the one that is familiar to them.

I think it's better to just treat the Agent as an opaque datatype and pass it through to the underlying request library. This way people can also use experimental Agent implementations should they wish to, and we are none the wiser.

switching to library that does not allow providing Agent

I'm not sure we'd ever do this, there would be no way to control connection pooling in node, since it's all done by Agents. Unless I suppose the library implemented it's own HTTP client over raw sockets but the chances of this seem small.

@achingbrain achingbrain merged commit fe93ba0 into master Jan 13, 2021
@achingbrain achingbrain deleted the feat/allow-passing-agent-to-http-client branch January 13, 2021 11:46
achingbrain added a commit that referenced this pull request Jan 14, 2021
Follows on from #3474 and allows using http.Agents with node.js to
control the behaviour of the underlying node http client.
achingbrain added a commit that referenced this pull request Jan 14, 2021
Follows on from #3474 and allows using http.Agents with node.js to
control the behaviour of the underlying node http client.
achingbrain added a commit that referenced this pull request Jan 20, 2021
Fixes regression introduced in #3474 - if we make https requests,
use a https agent.
achingbrain added a commit that referenced this pull request Jan 20, 2021
Fixes regression introduced in #3474 - if we make https requests,
use a https agent.
SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022
Right now no `http.Agent` is used for requests made using the http client in node, which means each request opens a new connection which can end up hitting process resource limits which means connections get dropped.

The change here sets a default `http.Agent` with a `keepAlive: true` and `maxSockets` of 6 which is consistent with [browsers](https://tools.ietf.org/html/rfc2616#section-8.1.4) and [native apps](https://developer.apple.com/documentation/foundation/nsurlsessionconfiguration/1407597-httpmaximumconnectionsperhost?language=objc).

The user can override the agent passed to the `ipfs-http-client` constructor to restore the previous functionality:

```js
const http = require('http')
const createClient = require('ipfs-http-client')

const client = createClient({
  url: 'http://127.0.0.1:5002',
  agent: new http.Agent({
    keepAlive: false,
    maxSockets: Infinity
  })
})
```

Refs: #3464
SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022
Follows on from #3474 and allows using http.Agents with node.js to
control the behaviour of the underlying node http client.
SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022
Fixes regression introduced in #3474 - if we make https requests,
use a https agent.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants