Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request flood in web browser #12

Closed
lidel opened this issue Jul 12, 2019 · 6 comments · Fixed by #16
Closed

Request flood in web browser #12

lidel opened this issue Jul 12, 2019 · 6 comments · Fixed by #16

Comments

@lidel
Copy link
Member

lidel commented Jul 12, 2019

Problem

I've been testing delegated routing in browser context, namely with js-ipfs embedded in Brave (ipfs/ipfs-companion#716).

It produces pretty intense list of pending requests when browsing /ipns/tr.wikipedia-on-ipfs.org 🎠 💦

pending-swarm-connect-2019-07-12--22-35-50

swarm-connect-content-routing-2019-07-12--22-38-42

Modern browsers limit the number of concurrent requests to ~6 per hostname.
Everything over the limit lands in "pending" state.

On some pages it takes a dozen of seconds for the browser to finish a pending list like the one above (it was multiple screens), and while it happens nothing else works.

When run in regular page, this flood of swarm connect results in slow browsing experience (nothing loads, "waiting for available socket" displayed in bottom left).

In my case (Brave + chrome.sockets), coupled with regular preload requests it produced more severe net::ERR_INSUFFICIENT_RESOURCES:

net-insufficient-resources-2019-07-12--21-37-15

Fix

My initial idea would be to limit the number of concurrent requests using something like p-queue around these lines:

const results = await Promise.all(
addrs.map((addr) => {
return this.swarm.connect(addr.toString()).catch(() => {})

I'll prepare PR with this.

@lidel
Copy link
Member Author

lidel commented Jul 22, 2019

Note to self:

  • (while testing with public delegate servers) when we throttle the number of parallel requests DoS protection gets triggered at Nginx
  • we don't cache successful swarm connect: requests are sent over and over again, but could cache it as there is an expectation that connection is alive (at least for 1minute..)
    • this will decrease the number of requests significantly, as swarm connect is the main offender

@lidel
Copy link
Member Author

lidel commented Jul 22, 2019

Update: throttling/caching helps, but is not enough. Some requests can take a very long time and will suffocate request queue because they have no built-in timeout (namely /api/v0/refs).
This is a separate issue: filled #15

@alanshaw
Copy link
Member

@lidel they are mostly calls to swarm.connect - I'm not sure they are even needed. See the conversation here: #14 (comment)

@lidel
Copy link
Member Author

lidel commented Jul 23, 2019

@alanshaw yeah, that confirms what I'm seeing: swarm connect from delegate to js-ipfs always succeeds, no matter what multiaddr was used:

2019-07-23--17-01-34
2019-07-23--17-00-53

AFAIK autorelay feature is off on bootstrap nodes. However bootstraps in this module do not come from js-ipfs (param is undefined as js-ipfs does not pass bootstraps)

This means p2p-circuits are never used(!) however it works because node0.preload.ipfs.io is reusing already existing connection that my node established with it because its one of bootstrap nodes.

(go-ipfs ignores multiaddr if it already has a working connection with same peerId)

This means we could either remove swarm connect entirely (js-ipfs will maintain connection to delegate/bootstrap node, no need to force it), or finish early, after first success (decreasing number of requests from one per bootstrap, to a single one).

Given the docs explicitly state that delegate needs to also be a bootstrap node (libp2p/js-libp2p#371), I'd vote for removal of swarm connect call and relatec code.

lidel added a commit to lidel/js-libp2p-delegated-peer-routing that referenced this issue Jul 24, 2019
All HTTP requests made by this module are sent to the same delegate
host. Browsers throttle the number of concurrent requests per hostname,
right now it is 6 per host, which suffocates the use of delegate and
blocking it from being used for preload or delegated content routing.

This introduces a task queue that limits the number of concurrent
requests, making it safe to run in browser context.

See also:
libp2p/js-libp2p-delegated-content-routing#12

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to lidel/js-libp2p-delegated-content-routing that referenced this issue Jul 24, 2019
All HTTP requests made by this module are sent to the same delegate
host. Browsers throttle the number of concurrent requests per hostname,
right now it is 6 per host, which suffocates the use of delegate and
blocking it from being used for preload or delegated peer routing.

This change introduces task queues that limit the number of concurrent
requests, making it safe to run in browser context.

Optimizations:
- preload calls via `refs` are known to take time,
  so we limit them to max two at a time
- swarm.connect was the main offender (causing multiple requests to delegate),
  we now check if connection is already present and cache result for 1 minute
  removing most of redundant http requests
- hostname of default delegate is changed

Context: libp2p#12
Closes libp2p#12

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to lidel/js-libp2p-delegated-content-routing that referenced this issue Jul 24, 2019
All HTTP requests made by this module are sent to the same delegate
host. Browsers throttle the number of concurrent requests per hostname,
right now it is 6 per host, which suffocates the use of delegate and
blocking it from being used for preload or delegated peer routing.

This change introduces task queues that limit the number of concurrent
requests, making it safe to run in browser context.

Optimizations:
- preload calls via `refs` are known to take time,
  so we limit them to max two at a time
- swarm.connect was the main offender (causing multiple requests to delegate),
  we now check if connection is already present and cache result for 1 minute
  removing most of redundant http requests
- hostname of default delegate is changed

Context: libp2p#12
Closes libp2p#12

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@jacobheun
Copy link
Contributor

Catching up on this now, sorry for the delay.

Given the docs explicitly state that delegate needs to also be a bootstrap node (libp2p/js-libp2p#371), I'd vote for removal of swarm connect call and relatec code.

Yes, we should remove this code. Right now we're telling the delegate to connect to us through every bootstrap as relays, which isn't going to work as they're not relays. I think this code may have made the assumption that we could be using a delegate that's not publicly dialable, which is wrong.

Getting the Priority Peer or (Friend Peer) functionality in place and documenting that users should have their delegate node as a priority peer should be sufficient for us just calling refs on the delegate.

All the bootstrap node stuff just needs to go away.

lidel added a commit to lidel/js-libp2p-delegated-content-routing that referenced this issue Jul 24, 2019
This removes all code that caused swarm connect calls from delegate to
self. It was not needed: delegate is one of bootstrap nodes,
so we are always connected to it.

libp2p#12 (comment)

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
Copy link
Member Author

lidel commented Jul 24, 2019

@jacobheun neat! I removed bootstrap node stuff as part of #16

jacobheun pushed a commit that referenced this issue Jul 24, 2019
* fix: limit concurrent HTTP requests

All HTTP requests made by this module are sent to the same delegate
host. Browsers throttle the number of concurrent requests per hostname,
right now it is 6 per host, which suffocates the use of delegate and
blocking it from being used for preload or delegated peer routing.

This change introduces task queues that limit the number of concurrent
requests, making it safe to run in browser context.

Optimizations:
- preload calls via `refs` are known to take time,
  so we limit them to max two at a time
- swarm.connect was the main offender (causing multiple requests to delegate),
  we now check if connection is already present and cache result for 1 minute
  removing most of redundant http requests
- hostname of default delegate is changed

Context: #12
Closes #12

* refactor: remove the need for swarm connect

This removes all code that caused swarm connect calls from delegate to
self. It was not needed: delegate is one of bootstrap nodes,
so we are always connected to it.

#12 (comment)

* chore: remove unused dependencies

We no longer need those (removed calls to swarm.connect)

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
jacobheun pushed a commit to libp2p/js-libp2p-delegated-peer-routing that referenced this issue Jul 24, 2019
All HTTP requests made by this module are sent to the same delegate
host. Browsers throttle the number of concurrent requests per hostname,
right now it is 6 per host, which suffocates the use of delegate and
blocking it from being used for preload or delegated content routing.

This introduces a task queue that limits the number of concurrent
requests, making it safe to run in browser context.

See also:
libp2p/js-libp2p-delegated-content-routing#12

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to lidel/js-ipfs that referenced this issue Jul 26, 2019
Adds 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.

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

This is similar to:
libp2p/js-libp2p-delegated-content-routing#12

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to lidel/js-ipfs that referenced this issue Aug 12, 2019
Adds 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.

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

This is similar to:
libp2p/js-libp2p-delegated-content-routing#12

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to lidel/js-ipfs that referenced this issue Aug 27, 2019
Adds 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.

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

This is similar to:
libp2p/js-libp2p-delegated-content-routing#12

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to lidel/js-ipfs that referenced this issue Aug 28, 2019
Adds 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.

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

This is similar to:
libp2p/js-libp2p-delegated-content-routing#12

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to lidel/js-ipfs that referenced this issue Sep 4, 2019
Adds 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.

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

This is similar to:
libp2p/js-libp2p-delegated-content-routing#12

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to lidel/js-ipfs that referenced this issue Sep 5, 2019
Adds 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.

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

This is similar to:
libp2p/js-libp2p-delegated-content-routing#12

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
hugomrdias pushed a commit to ipfs/js-ipfs that referenced this issue Sep 10, 2019
Adds 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.

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

This is similar to:
libp2p/js-libp2p-delegated-content-routing#12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants