fix: limit concurrent HTTP requests in browser #2304

lidel · 2019-07-26T14:50:39Z

This PR solves request floods like this:

It adds a limit of concurrent HTTP requests sent to remote API
by dns and preload calls when running in web browser contexts.

Browsers limit connections per host (~6). This change mitigates the problem of expensive and long running calls of one type exhausting connection pool for other uses.
This is similar to problems described in libp2p/js-libp2p-delegated-content-routing#12

This PR additionally limits the number of DNS lookup calls by introducing time-bound cache with eviction rules following what browser already do.

TODO

add queue for remote HTTP API calls
add cache for remote /api/v0/dns in browser
pass dnslink tests in browser
pass preload tests in browser
- disable unnecessary preloads when running interface-ipfs-core tests with process.env.NODE_ENV === 'test'
pass electron tests
figure out addFromURL tests

cc ipfs/ipfs-companion#716

src/core/runtime/preload-browser.js

src/core/runtime/dns-browser.js

hugomrdias · 2019-07-30T15:36:39Z

src/core/runtime/dns-browser.js

  Object.keys(opts).forEach(prop => {
    url += `&${encodeURIComponent(prop)}=${encodeURIComponent(opts[prop])}`
  })

-  self.fetch(url, { mode: 'cors' })
+  _httpQueue.add(() => fetch(url, { mode: 'cors' })


i would use https://github.com/sindresorhus/ky (check this discussion ipfs-inactive/js-ipfs-http-client#1059 (comment))

it will simplify the above lines too and you can specific retries

Took a stab at ky in 410ac10. Code is much simpler now. Thoughts?

We were using fetch for preload and dns in browser, however addFromURL was handled via custom code on top of node-fetch. I removed custom code around fetch and used ky-universal everywhere for consistency.

All of them benefit from retries provided by ky out of the box.
Question: should we hardcode timeout and retry, or go with defaults?

we will probably create an ky instance to be used everywhere but for now just leave it hardcoded

hugomrdias

i would have preferred a promise first approach like this https://github.com/ipfs/js-ipfs/blob/156125535213836cda58249576f6dbfa2eeac039/src/core/components/resolve.js but i will not block this PR because of it !!

Another suggestion is to use ky.get and ky.post to be more explicit and easier to identify the HTTP operation.

src/core/components/files-regular/add-from-url.js

src/core/runtime/dns-browser.js

src/core/runtime/preload-browser.js

src/core/runtime/dns-browser.js

hugomrdias · 2019-07-31T19:49:43Z

src/core/runtime/dns-browser.js

+  hooks: {
+    afterResponse: [
+      async response => {
+        const query = new URLSearchParams(new URL(response.url).search).toString()


the response doesn't have the searchParams already ?

afaik no, when I checked it only had .url (string)

hugomrdias · 2019-07-31T20:02:04Z

damn ky doesnt support returning a Response from beforeRequest to skip the http request entirely and fetch from cache.

sindresorhus/ky#157

lidel · 2019-08-01T13:45:42Z

Preload tests fail in browser because preload is enabled in browser context by default and while js-ipfs tests disable preload in tests, interface-ipfs-core does not.

This means interface-ipfs-core tests trigger dozens of slow/hanging requests to nodeX.preload.iofs.io/api/v0/refs and we execute only 4 at a time. By the time we do preload tests queue is long and it does not finish before timeout.

I believe we should detect js-ipfs is running in test environment (process.env.NODE_ENV === 'test' ?) and make preload opt-in in that environment, even in browser. That way we would still be able to tests preload (by explicitly providing preload nodes), but we won't have overhead when running interface-ipfs-core tests.

hugomrdias · 2019-08-01T14:20:44Z

This basically is telling us to "really" solve this problem lol, with http2, http cache and making refs endpoint faster

Still if you want to do this workaround make sure NODE_ENV is properly set in browsers and electron if not file an issue in aegir and i'll fix it.

lidel · 2019-08-01T20:55:17Z

We really don't need preload calls in tests other than tests of preload itself.

76ae81c disables preload by default when NODE_ENV is test:

We only care about preload in... preload tests: we use custom localhost server there and enable preload via explicit config, so the change does not impact those tests.
This means there is no need to enable it by default in test environment. Running preload call in every test of interface-js-ipfs-core was just unecessary overhead.
NODE_ENV is set ok when running browser tests, so no aegir fix is necessary.

Remaining thing to fix is addFromURL test with HTTPS under electron renderer:

new	old

lidel · 2019-08-02T12:50:16Z

The problem was that node-fetch was not used in electron-renderer context:

ky-universal won't use node-fetch because electron-renderer already has
global.fetch defined, and we can't use the one from electron-renderer as
it returns different errors and makes HTTPS mocking impossible in tests

As a temporary workaround, 8f41347 fixes the problem in userland by assigning node-fetch to global.fetch before ky-universal is imported.

We should solve this upstream somewhow. So far options I see are:

(A) PR ky-universal and ensure node-fetch is used in electron-renderer context
(B) PR aegir and ensure node-fetch overrides global.fetch provided by electron-renderer
(C) move workaround from 8f41347 to ipfs-utils/src/iso-ky and use it instead of ky-universal directly?

@hugomrdias thoughts?

hugomrdias · 2019-08-02T12:52:54Z

I would like to test with the new electron 6 before deciding

lidel · 2019-08-02T13:16:48Z

@hugomrdias same errors in 6.0.0:

lidel · 2019-08-12T11:54:57Z

Updates

rebased on top of v0.37.0
electron tests got fixed for now by explicitly using node-fetch in electron-renderer

On `addFromURL` tests

Turns out js-ipfs already skips addFromURL tests in browsers:

js-ipfs/test/core/interface.spec.js

Lines 67 to 68 in fc76875

    
           name: 'addFromURL', 
        
           reason: 'Not designed to run in the browser'

This is because karma running interface-js-ipfs-core tests in browser context is unable to start HTTP(S) server. We may be able to make it work with something like karma-server-side, but it sounds like something we need to add to interface-js-ipfs-core or maybe aegir directly.

@hugomrdias any thoughts on this?

When we have addFromURL tests working in web browser, then we can revisit electron errors (we probably need to rewrite node-centric tests, and the discrepancies will not matter anymore).

This switches to 0.2.x versions of delegate modules which work correctly with js-libp2p + wip [1] fix for js-ipfs that caches DNS records for 1 minute, greatly reducing the HTTP request overhead to remote APIs. [1]: ipfs/js-ipfs#2304

* feat(brave): delegated peers and content routing This enables delegated routers in embedded js-ipfs running in Brave. Coupled with preload, this gives us basic file sharing functionality back, until we have real p2p transport, native DHT etc. * fix: callback-based delgates + DNS caching This switches to 0.2.x versions of delegate modules which work correctly with js-libp2p + wip [1] fix for js-ipfs that caches DNS records for 1 minute, greatly reducing the HTTP request overhead to remote APIs. [1]: ipfs/js-ipfs#2304

hugomrdias · 2019-08-26T14:49:19Z

for reference

hugomrdias: ok so we need to find a way to test https
or just skip the https tests
not the others
lidel: I'd skip https in browser
hugomrdias: ok cool
remove this

js-ipfs/test/core/interface.spec.js

Line 67 in fc76875

name: 'addFromURL',

and skip inside the tests with ipfs-utils/src/env.js
do you agree ?
lidel: yup

alanshaw · 2019-08-27T08:54:32Z

@lidel what's the current status of this PR - still draft?

lidel · 2019-08-27T09:52:42Z

@alanshaw still a draft. Remaining work here is to figure out addFromURL tests in the browser (interface-js-ipfs-core).

I took a break from this PR after my vacation, but will rebase and try to get it ready for review this week.

alanshaw · 2019-08-27T10:15:50Z

Just incase this is helpful, you could spin up a server to serve some content via aegir hooks:

js-ipfs/.aegir.js

Line 27 in c67b8a5

hooks: {

Maybe the server accepts a querystring that is the fixture you want it to return? Then in tests you can call it and assert the correct content was added.

Won't work with HTTPS though, but only HTTP is better than nothing.

we need this version for isTest check License: MIT Signed-off-by: Marcin Rataj <[email protected]>

lidel · 2019-09-06T16:49:59Z

@alanshaw switched to ipfs-utils v0.2.0 in ad65329 should be ok to merge now
(ci seems to fail on windows due to unrelated reasons)

alanshaw · 2019-09-06T17:09:05Z

hmm, tests are failing in windows and both browsers and electron. Something is not right here! The addFromURL tests are failing in windows and they have been changed in interface-ipfs-core for this PR.

lidel · 2019-09-06T22:03:20Z

oh boy, this PR is cursed 👀

@alanshaw electron was a random one, but remaining Windows issue was triggered by parallel ipfs.add of the same data. I fixed it in ipfs-inactive/interface-js-ipfs-core#523 ← needs to be merged & released first.

License: MIT Signed-off-by: Marcin Rataj <[email protected]>

lidel · 2019-09-09T10:35:11Z

Resolved conflicts + re-enabled tests for #2379
Windows tests failed due to ipfs-inactive/interface-js-ipfs-core#523 (comment) (should be fixed now, waiting for ci)

hugomrdias · 2019-09-09T11:31:59Z

This should be ready to merge now.

ipfs/js-ipfs#2304

achingbrain

A couple of small nits but this is great.

achingbrain · 2019-09-10T09:31:27Z

src/core/components/files-regular/add-from-url.js

+  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())


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

achingbrain · 2019-09-10T09:39:56Z

src/core/runtime/dns-browser.js

+  throw new Error(response.Message)
+}
+
+module.exports = (fqdn, opts = {}, cb) => {


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.

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.

src/core/runtime/preload-browser.js

License: MIT Signed-off-by: Marcin Rataj <[email protected]>

lidel · 2019-09-10T18:14:05Z

Woohoo merged 🎉

ipfs/js-ipfs#2304

This switch to js-ipfs before PR-2379 ipfs/js-ipfs#2379 switched ipfs.add to ipfs._addAsyncIterator and it broke /api/v0/add exposed by embedded js-ipfs in Brave. It seems old polyfills are no longer enough. Real fix requires more time to investigate, so for now we switch to version before js-ipfs/PR-2379. Used js-ipfs commit is from ipfs/js-ipfs#2304 before it was rebased on top of master after PR-2379, making it the last safe version. Real fix will be tracked in #757

typhu-xyz · 2020-05-16T18:21:31Z

I'm encountering this problem in latest ipfs-http-client - has this been dropped during latest development?

hugomrdias · 2020-05-18T10:55:20Z

I'm encountering this problem in latest ipfs-http-client - has this been dropped during latest development?

Can you explain further ? this was a problem in js-ipfs not http-client

typhu-xyz · 2020-05-18T22:06:26Z

@hugomrdias I was playing around with orbitdb + ipfs-http-client in web browser (connected to my local go-ipfs). I found that when I subscribe to a lot of datastores, orbitdb's periodic fetches / pubsub on each datastore effectively use up the concurrent HTTP request limit and stall the entire pipeline of requests, just like what's shown in the screenshot in the PR. This behavior seems to be a result from https://github.com/ipfs-shipyard/ipfs-pubsub-1on1.

When I saw this PR, I thought this might be something that addresses my issue, but you are right - I missed that this PR was for js-ipfs. It wouldn't make much sense for ipfs-http-client in fact.

hugomrdias · 2020-05-19T12:57:53Z

can you make another issue with further info maybe a repo we can clone please ?

typhu-xyz · 2020-05-19T21:33:24Z

I've decided to drop OrbitDB for now, so maybe won't see the issue anymore. But if I manage to reproduce it in a consistent manner will log a separate issue like you suggested. Thanks!

lidel requested review from hugomrdias and alanshaw and removed request for alanshaw and hugomrdias July 26, 2019 21:56

hugomrdias approved these changes Jul 30, 2019

View reviewed changes

lidel force-pushed the fix/dns-cache-and-http-throttling-in-browser branch from 19dcf6c to 410ac10 Compare July 30, 2019 21:44

hugomrdias suggested changes Jul 31, 2019

View reviewed changes

lidel force-pushed the fix/dns-cache-and-http-throttling-in-browser branch 4 times, most recently from 7d82d34 to 070e205 Compare July 31, 2019 19:40

hugomrdias suggested changes Jul 31, 2019

View reviewed changes

lidel force-pushed the fix/dns-cache-and-http-throttling-in-browser branch from 8f41347 to 6fa8f88 Compare August 12, 2019 11:21

lidel mentioned this pull request Aug 12, 2019

Embedded JS-IPFS in Brave (experiment) ipfs/ipfs-companion#716

Closed

52 tasks

alanshaw mentioned this pull request Aug 27, 2019

⚡️ v0.38.0 RELEASE 🚀 #2396

Closed

66 tasks

lidel force-pushed the fix/dns-cache-and-http-throttling-in-browser branch from 6fa8f88 to 25d3144 Compare August 27, 2019 09:58

chore: ipfs-utils v0.2.0

ad65329

we need this version for isTest check License: MIT Signed-off-by: Marcin Rataj <[email protected]>

lidel mentioned this pull request Sep 6, 2019

fix(ci): avoid fs clash during ipfs.add* on Windows ipfs-inactive/interface-js-ipfs-core#523

Closed

alanshaw mentioned this pull request Sep 7, 2019

Update interface-ipfs-core to the latest version 🚀 #2430

Closed

Merge branch 'master' into fix/dns-cache-and-http-throttling-in-browser

11ab304

License: MIT Signed-off-by: Marcin Rataj <[email protected]>

lidel force-pushed the fix/dns-cache-and-http-throttling-in-browser branch from 493a00b to 11ab304 Compare September 9, 2019 10:24

chore: unskip .add tests

ddd49ce

License: MIT Signed-off-by: Marcin Rataj <[email protected]>

lidel force-pushed the fix/dns-cache-and-http-throttling-in-browser branch from fade339 to ddd49ce Compare September 9, 2019 10:33

lidel added a commit to ipfs/ipfs-companion that referenced this pull request Sep 9, 2019

chore: update js-ipfs

350f880

ipfs/js-ipfs#2304

lidel requested a review from achingbrain September 9, 2019 18:47

achingbrain suggested changes Sep 10, 2019

View reviewed changes

style: remove underscore

2e82b2a

License: MIT Signed-off-by: Marcin Rataj <[email protected]>

achingbrain approved these changes Sep 10, 2019

View reviewed changes

hugomrdias merged commit cf38aea into ipfs:master Sep 10, 2019

lidel deleted the fix/dns-cache-and-http-throttling-in-browser branch September 10, 2019 17:59

lidel added a commit to ipfs/ipfs-companion that referenced this pull request Sep 11, 2019

chore: update js-ipfs

4188058

ipfs/js-ipfs#2304

lidel mentioned this pull request Sep 12, 2019

Brave: regression in embedded /api/v0/add ipfs/ipfs-companion#757

Closed

achingbrain mentioned this pull request Sep 25, 2019

⚡️ v0.39.0 RELEASE 🚀 #2489

Closed

52 tasks

snyk-bot mentioned this pull request Dec 6, 2019

[Snyk] Upgrade ipfs from 0.37.1 to 0.39.0 dyna-dot/orbit-web#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: limit concurrent HTTP requests in browser #2304

fix: limit concurrent HTTP requests in browser #2304

lidel commented Jul 26, 2019 •

edited

Loading

hugomrdias Jul 30, 2019

lidel Jul 30, 2019 •

edited

Loading

hugomrdias Jul 31, 2019

hugomrdias left a comment

hugomrdias Jul 31, 2019

lidel Jul 31, 2019 •

edited

Loading

hugomrdias commented Jul 31, 2019 •

edited

Loading

lidel commented Aug 1, 2019

hugomrdias commented Aug 1, 2019

lidel commented Aug 1, 2019 •

edited

Loading

lidel commented Aug 2, 2019 •

edited

Loading

hugomrdias commented Aug 2, 2019

lidel commented Aug 2, 2019 •

edited

Loading

lidel commented Aug 12, 2019 •

edited

Loading

hugomrdias commented Aug 26, 2019

alanshaw commented Aug 27, 2019

lidel commented Aug 27, 2019 •

edited

Loading

alanshaw commented Aug 27, 2019

lidel commented Sep 6, 2019 •

edited

Loading

alanshaw commented Sep 6, 2019

lidel commented Sep 6, 2019 •

edited

Loading

lidel commented Sep 9, 2019

hugomrdias commented Sep 9, 2019

achingbrain left a comment

achingbrain Sep 10, 2019

achingbrain Sep 10, 2019

lidel Sep 10, 2019

lidel commented Sep 10, 2019 •

edited

Loading

typhu-xyz commented May 16, 2020

hugomrdias commented May 18, 2020

typhu-xyz commented May 18, 2020

hugomrdias commented May 19, 2020

typhu-xyz commented May 19, 2020

fix: limit concurrent HTTP requests in browser #2304

fix: limit concurrent HTTP requests in browser #2304

Conversation

lidel commented Jul 26, 2019 • edited Loading

hugomrdias Jul 30, 2019

Choose a reason for hiding this comment

lidel Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

hugomrdias Jul 31, 2019

Choose a reason for hiding this comment

hugomrdias left a comment

Choose a reason for hiding this comment

hugomrdias Jul 31, 2019

Choose a reason for hiding this comment

lidel Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

hugomrdias commented Jul 31, 2019 • edited Loading

lidel commented Aug 1, 2019

hugomrdias commented Aug 1, 2019

lidel commented Aug 1, 2019 • edited Loading

lidel commented Aug 2, 2019 • edited Loading

hugomrdias commented Aug 2, 2019

lidel commented Aug 2, 2019 • edited Loading

lidel commented Aug 12, 2019 • edited Loading

Updates

On addFromURL tests

hugomrdias commented Aug 26, 2019

alanshaw commented Aug 27, 2019

lidel commented Aug 27, 2019 • edited Loading

alanshaw commented Aug 27, 2019

lidel commented Sep 6, 2019 • edited Loading

alanshaw commented Sep 6, 2019

lidel commented Sep 6, 2019 • edited Loading

lidel commented Sep 9, 2019

hugomrdias commented Sep 9, 2019

achingbrain left a comment

Choose a reason for hiding this comment

achingbrain Sep 10, 2019

Choose a reason for hiding this comment

achingbrain Sep 10, 2019

Choose a reason for hiding this comment

lidel Sep 10, 2019

Choose a reason for hiding this comment

lidel commented Sep 10, 2019 • edited Loading

typhu-xyz commented May 16, 2020

hugomrdias commented May 18, 2020

typhu-xyz commented May 18, 2020

hugomrdias commented May 19, 2020

typhu-xyz commented May 19, 2020

lidel commented Jul 26, 2019 •

edited

Loading

lidel Jul 30, 2019 •

edited

Loading

lidel Jul 31, 2019 •

edited

Loading

hugomrdias commented Jul 31, 2019 •

edited

Loading

lidel commented Aug 1, 2019 •

edited

Loading

lidel commented Aug 2, 2019 •

edited

Loading

lidel commented Aug 2, 2019 •

edited

Loading

lidel commented Aug 12, 2019 •

edited

Loading

On `addFromURL` tests

lidel commented Aug 27, 2019 •

edited

Loading

lidel commented Sep 6, 2019 •

edited

Loading

lidel commented Sep 6, 2019 •

edited

Loading

lidel commented Sep 10, 2019 •

edited

Loading