Skip to content

fix: limit concurrent HTTP requests (backport for 0.2.x)#17

Merged
jacobheun merged 3 commits into
libp2p:0.2.xfrom
lidel:fix/backport-of-http-throttling
Jul 29, 2019
Merged

fix: limit concurrent HTTP requests (backport for 0.2.x)#17
jacobheun merged 3 commits into
libp2p:0.2.xfrom
lidel:fix/backport-of-http-throttling

Conversation

@lidel
Copy link
Copy Markdown
Member

@lidel lidel commented Jul 26, 2019

This is a backport of the fix from #16 that works with 0.2.x.

The goal is to include the fix in latest js-ipfs release (without waiting for async/await changes to be propagated to js-libp2p)

cc @alanshaw @jacobheun

This is a backport of the fix from
libp2p#16
that works with 0.2.x

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
Comment thread src/index.js Outdated
this.dht.findProvs(key.toString(), {
timeout: `${options.maxTimeout}ms` // The api requires specification of the time unit (s/ms)
}, callback)
).catch(callback)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know exactly how promisify-es6 works under the hood, does it call callback as well as return a promise? I would have thought it did one or the other.

Maybe we can make this a bit easier to understand as well as eliminate any doubt?

    this._httpQueue.add(() =>
      this.dht.findProvs(key.toString(), {
        timeout: `${options.maxTimeout}ms` // The api requires specification of the time unit (s/ms)
      })
    ).then(res => callback(null, res), callback)

Copy link
Copy Markdown
Member Author

@lidel lidel Jul 26, 2019

Choose a reason for hiding this comment

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

iiuc if callback is present, promisify-es6 will call function as-is; if not, it will return a Promise that injects callback as the last argument before calling the thing

promisify-es6/index.js#L31-L32

Should work the same, so I've switched to your version in 91dcbb7

Comment thread src/index.js Outdated
], (err) => callback(err))
this._httpQueueRefs.add(() =>
this.refs(key.toString(), { recursive: false }, (err, res) => callback(err))
).catch(callback)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's odd: last test (provide) stop passing if I move callback call to the outside:

    this._httpQueueRefs.add(() =>
      this.refs(key.toString(), { recursive: false })
    ).then(res => callback(null, res), callback)

makes tests hang:

  6 passing (22s)
  1 failing

  1) DelegatedContentRouting
       provide
         should be able to register as a content provider to the delegate node:
     Error: Timeout of 20000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/lidel/project/js-libp2p-delegated-content-routing/test/index.spec.js)

Bit late on my end, so I can't tell if it is a bug somewhere, or just me being tired.
Thoughts?

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel changed the title fix: limit concurrent HTTP requests fix: limit concurrent HTTP requests (backport for 0.2.x) Jul 26, 2019
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@alanshaw alanshaw requested a review from jacobheun July 26, 2019 20:03
Copy link
Copy Markdown
Contributor

@jacobheun jacobheun 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, I'll get a release out for 2.x

@jacobheun jacobheun merged commit 2ea1334 into libp2p:0.2.x Jul 29, 2019
@jacobheun
Copy link
Copy Markdown
Contributor

0.2.4 is on the webs!

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 this pull request may close these issues.

3 participants