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

fix: limit concurrent HTTP requests in browser #2304

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
5c5f891
fix: limit concurrent HTTP requests in browser
lidel Jul 26, 2019
45465ff
fix: undefined in TLUR.get
lidel Jul 26, 2019
7e422e9
fix: dns cache per domain+opts
lidel Jul 27, 2019
7171981
refactor: simplify error handling
lidel Jul 30, 2019
6c404ea
chore: increase bundlesize maxSize
lidel Jul 30, 2019
3ea284d
refactor: call callbacks inside setImmediate
lidel Jul 30, 2019
d5fd304
refactor: replace custom fetch with ky-universal
lidel Jul 30, 2019
a6d09c1
refactor: simplify http client code with ky
lidel Jul 31, 2019
1999de7
refactor: promise first addFromURL
lidel Jul 31, 2019
c4d0a83
refactor: promise first dns in browser
lidel Jul 31, 2019
a074cbb
fix(ci): disable preload when NODE_ENV is 'test'
lidel Aug 1, 2019
4af8560
fix: use node-fetch in electron-renderer
lidel Aug 2, 2019
b5038ec
fix: enable tests of addFromURL in browser
lidel Aug 27, 2019
857769a
refactor: run echo-http-server via aegir
lidel Aug 30, 2019
79ae97f
refactor: addressing review
lidel Sep 4, 2019
0f6b543
refactor: ky v0.13.0
lidel Sep 4, 2019
e29e42b
fix: call callback via setImmediate
lidel Sep 4, 2019
c239abd
fix(ci): skip tests without implementation
lidel Sep 5, 2019
e7534ff
refactor: ipfsdServer start/stop
lidel Sep 5, 2019
62dbdc3
refactor: ipfs-utils/src/env/isTest
lidel Sep 5, 2019
e49143e
Merge branch 'master' into fix/dns-cache-and-http-throttling-in-browser
lidel Sep 5, 2019
af208c3
Merge branch 'master' into fix/dns-cache-and-http-throttling-in-browser
lidel Sep 6, 2019
ad65329
chore: ipfs-utils v0.2.0
lidel Sep 6, 2019
11ab304
Merge branch 'master' into fix/dns-cache-and-http-throttling-in-browser
lidel Sep 9, 2019
ddd49ce
chore: unskip .add tests
lidel Sep 9, 2019
2e82b2a
style: remove underscore
lidel Sep 10, 2019
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
2 changes: 1 addition & 1 deletion .aegir.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const preloadNode = MockPreloadNode.createNode()
const echoServer = EchoServer.createServer()

module.exports = {
bundlesize: { maxSize: '689kB' },
bundlesize: { maxSize: '692kB' },
webpack: {
resolve: {
mainFields: ['browser', 'main'],
Expand Down
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
"./src/core/runtime/add-from-fs-nodejs.js": "./src/core/runtime/add-from-fs-browser.js",
"./src/core/runtime/config-nodejs.js": "./src/core/runtime/config-browser.js",
"./src/core/runtime/dns-nodejs.js": "./src/core/runtime/dns-browser.js",
"./src/core/runtime/fetch-nodejs.js": "./src/core/runtime/fetch-browser.js",
"./src/core/runtime/libp2p-nodejs.js": "./src/core/runtime/libp2p-browser.js",
"./src/core/runtime/libp2p-pubsub-routers-nodejs.js": "./src/core/runtime/libp2p-pubsub-routers-browser.js",
"./src/core/runtime/preload-nodejs.js": "./src/core/runtime/preload-browser.js",
Expand Down Expand Up @@ -123,6 +122,8 @@
"it-to-stream": "^0.1.1",
"just-safe-set": "^2.1.0",
"kind-of": "^6.0.2",
"ky": "~0.13.0",
"ky-universal": "~0.3.0",
"libp2p": "~0.26.1",
"libp2p-bootstrap": "~0.9.3",
"libp2p-crypto": "~0.16.0",
Expand Down Expand Up @@ -151,7 +152,7 @@
"multicodec": "~0.5.5",
"multihashes": "~0.4.14",
"multihashing-async": "~0.6.0",
"node-fetch": "^2.3.0",
"p-queue": "^6.1.0",
"peer-book": "~0.9.0",
"peer-id": "~0.12.3",
"peer-info": "~0.15.0",
Expand Down
3 changes: 2 additions & 1 deletion src/cli/commands/daemon.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const os = require('os')
const toUri = require('multiaddr-to-uri')
const { ipfsPathHelp } = require('../utils')
const { isTest } = require('ipfs-utils/src/env')

module.exports = {
command: 'daemon',
Expand All @@ -27,7 +28,7 @@ module.exports = {
})
.option('enable-preload', {
type: 'boolean',
default: true
default: !isTest // preload by default, unless in test env
})
},

Expand Down
49 changes: 15 additions & 34 deletions src/core/components/files-regular/add-from-url.js
Original file line number Diff line number Diff line change
@@ -1,41 +1,22 @@
'use strict'

const { URL } = require('iso-url')
const fetch = require('../../runtime/fetch-nodejs')

module.exports = (self) => {
return async (url, options, callback) => {
if (typeof options === 'function') {
callback = options
options = {}
}

let files

try {
const parsedUrl = new URL(url)
const res = await fetch(url)

if (!res.ok) {
throw new Error('unexpected status code: ' + res.status)
}

// TODO: use res.body when supported
const content = Buffer.from(await res.arrayBuffer())
const path = decodeURIComponent(parsedUrl.pathname.split('/').pop())

files = await self.add({ content, path }, options)
} catch (err) {
if (callback) {
return callback(err)
}
throw err
}
const nodeify = require('promise-nodeify')
const { default: ky } = require('ky-universal')

module.exports = (ipfs) => {
const addFromURL = async (url, opts = {}) => {
const res = await ky.get(url)
const path = decodeURIComponent(new URL(res.url).pathname.split('/').pop())
const content = Buffer.from(await res.arrayBuffer())
Copy link
Member

Choose a reason for hiding this comment

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

One day we will use something like https://github.com/ipfs/js-ipfs-http-client/blob/master/src/lib/stream-to-iterable.js to stream the response.

Copy link
Member

Choose a reason for hiding this comment

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

That day is today. Well, yesterday. Since we merged #2379 you should be able to pass iterables to ipfs.add.

Copy link
Member Author

@lidel lidel Sep 10, 2019

Choose a reason for hiding this comment

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

@achingbrain seems that ky does not expose ReadableStream yet:
sindresorhus/ky#3 (comment)

To avoid blocking this PR, I'd keep this as-is and add streaming in separate PR, after support in ky lands.

return ipfs.add({ content, path }, opts)
}

if (callback) {
callback(null, files)
return (name, opts = {}, cb) => {
if (typeof opts === 'function') {
cb = opts
opts = {}
}

return files
return nodeify(addFromURL(name, opts), cb)
}
}
3 changes: 2 additions & 1 deletion src/core/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const Multiaddr = require('multiaddr')
const mafmt = require('mafmt')
const { struct, superstruct } = require('superstruct')
const { isTest } = require('ipfs-utils/src/env')

const { optional, union } = struct
const s = superstruct({
Expand Down Expand Up @@ -31,7 +32,7 @@ const configSchema = s({
enabled: 'boolean?',
addresses: optional(s(['multiaddr'])),
interval: 'number?'
}, { enabled: true, interval: 30 * 1000 }),
}, { enabled: !isTest, interval: 30 * 1000 }),
init: optional(union(['boolean', s({
bits: 'number?',
emptyRepo: 'boolean?',
Expand Down
3 changes: 2 additions & 1 deletion src/core/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const defaultRepo = require('./runtime/repo-nodejs')
const preload = require('./preload')
const mfsPreload = require('./mfs-preload')
const ipldOptions = require('./runtime/ipld-nodejs')
const { isTest } = require('ipfs-utils/src/env')

/**
* @typedef { import("./ipns/index") } IPNS
Expand All @@ -47,7 +48,7 @@ class IPFS extends EventEmitter {
start: true,
EXPERIMENTAL: {},
preload: {
enabled: true,
enabled: !isTest, // preload by default, unless in test env
addresses: [
'/dnsaddr/node0.preload.ipfs.io/https',
'/dnsaddr/node1.preload.ipfs.io/https'
Expand Down
77 changes: 53 additions & 24 deletions src/core/runtime/dns-browser.js
Original file line number Diff line number Diff line change
@@ -1,33 +1,62 @@
/* global self */
/* eslint-env browser */
'use strict'

module.exports = (domain, opts, callback) => {
const TLRU = require('../../utils/tlru')
const { default: PQueue } = require('p-queue')
const { default: ky } = require('ky-universal')
const nodeify = require('promise-nodeify')

// Avoid sending multiple queries for the same hostname by caching results
const cache = new TLRU(1000)
// TODO: /api/v0/dns does not return TTL yet: https://github.com/ipfs/go-ipfs/issues/5884
// However we know browsers themselves cache DNS records for at least 1 minute,
// which acts a provisional default ttl: https://stackoverflow.com/a/36917902/11518426
const ttl = 60 * 1000

// browsers limit concurrent connections per host,
// we don't want preload calls to exhaust the limit (~6)
const httpQueue = new PQueue({ concurrency: 4 })

// Delegated HTTP resolver sending DNSLink queries to ipfs.io
// TODO: replace hardcoded host with configurable DNS over HTTPS: https://github.com/ipfs/js-ipfs/issues/2212
const api = ky.create({
prefixUrl: 'https://ipfs.io/api/v0/',
hooks: {
afterResponse: [
async (input, options, response) => {
const query = new URL(response.url).search.slice(1)
const json = await response.json()
cache.set(query, json, ttl)
Copy link
Member

Choose a reason for hiding this comment

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

Only cache when response.ok?

Copy link
Member Author

@lidel lidel Sep 5, 2019

Choose a reason for hiding this comment

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

We can't: negative DNSLink result is returned as HTTP 500:
https://ipfs.io/api/v0/dns/arg=google.com
and we want to cache negatives too

}
]
}
})

const ipfsPath = (response) => {
if (response.Path) return response.Path
throw new Error(response.Message)
}

module.exports = (fqdn, opts = {}, cb) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be promise-first please? E.g. make this an async method but export a nodeified version. This will make future refactors to remove callbacks easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think its already done? the async method is inside:

const resolveDnslink = async (fqdn, opts = {}) => {
  ...
}
return nodeify(resolveDnslink(fqdn, opts), cb)

when we refactor to remove callbacks we just remove the wrapper.
Let me know if this should be handled some other way.

if (typeof opts === 'function') {
callback = opts
cb = opts
opts = {}
}
const resolveDnslink = async (fqdn, opts = {}) => {
const searchParams = new URLSearchParams(opts)
searchParams.set('arg', fqdn)

opts = opts || {}

domain = encodeURIComponent(domain)
let url = `https://ipfs.io/api/v0/dns?arg=${domain}`
// try cache first
const query = searchParams.toString()
if (!opts.nocache && cache.has(query)) {
const response = cache.get(query)
return ipfsPath(response)
}

Object.keys(opts).forEach(prop => {
url += `&${encodeURIComponent(prop)}=${encodeURIComponent(opts[prop])}`
})
// fallback to delegated DNS resolver
const response = await httpQueue.add(() => api.get('dns', { searchParams }).json())
return ipfsPath(response)
}

self.fetch(url, { mode: 'cors' })
.then((response) => {
return response.json()
})
.then((response) => {
if (response.Path) {
return callback(null, response.Path)
} else {
return callback(new Error(response.Message))
}
})
.catch((error) => {
callback(error)
})
return nodeify(resolveDnslink(fqdn, opts), cb)
}
3 changes: 0 additions & 3 deletions src/core/runtime/fetch-browser.js

This file was deleted.

2 changes: 0 additions & 2 deletions src/core/runtime/fetch-nodejs.js

This file was deleted.

18 changes: 8 additions & 10 deletions src/core/runtime/preload-browser.js
Original file line number Diff line number Diff line change
@@ -1,27 +1,25 @@
/* eslint-env browser */
'use strict'

const { default: PQueue } = require('p-queue')
const { default: ky } = require('ky-universal')
const debug = require('debug')

const log = debug('ipfs:preload')
log.error = debug('ipfs:preload:error')

// browsers limit concurrent connections per host,
// we don't want preload calls to exhaust the limit (~6)
const httpQueue = new PQueue({ concurrency: 4 })

module.exports = function preload (url, callback) {
log(url)

const controller = new AbortController()
const signal = controller.signal
const cb = () => setImmediate(callback) // https://github.com/ipfs/js-ipfs/pull/2304#discussion_r320700893

fetch(url, { signal })
.then(res => {
if (!res.ok) {
log.error('failed to preload', url, res.status, res.statusText)
throw new Error(`failed to preload ${url}`)
}
return res.text()
})
.then(() => callback())
.catch(callback)
httpQueue.add(() => ky.get(url, { signal })).then(cb, cb)

return {
cancel: () => controller.abort()
Expand Down
3 changes: 2 additions & 1 deletion src/utils/tlru.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ class TLRU {
this.lru.remove(key)
return undefined
}
return value.value
}
return value.value
return undefined
}

/**
Expand Down
3 changes: 0 additions & 3 deletions test/core/interface.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,6 @@ describe('interface-ipfs-core tests', function () {
}, {
name: 'addFromFs',
reason: 'Not designed to run in the browser'
}, {
name: 'addFromURL',
reason: 'Not designed to run in the browser'
}]
})

Expand Down