Skip to content
This repository has been archived by the owner on Apr 24, 2023. It is now read-only.

READY | alleviating browser throttling on WebRTC transport #145

Merged
merged 4 commits into from
Jun 1, 2018

Conversation

ya7ya
Copy link
Contributor

@ya7ya ya7ya commented May 29, 2018

This PR is related to ipfs/js-ipfs#1019 and ipfs/js-ipfs-bitswap#152 , it basically implements MAX_MESSAGE_SIZE on the webRTC transport to avoid being throttled by the browser.

however as you can see, it fails one of the tests. I would appreciate some help in debugging this since It's been frustrating.

I've tried to use pull-block but it didn't work (check the commented out code in dial)

note this also still requires some clean-up as you might have noticed 😄

cc. @diasdavid

@daviddias daviddias requested a review from pgte May 29, 2018 16:04
@pgte
Copy link
Contributor

pgte commented May 30, 2018

Looks like there's a race condition when ending the stream.
Perhaps you could try using the pump package to pipe the channel into block-stream and back, perhaps it will help you avoiding or finding the source of the issue?

@ya7ya ya7ya changed the title WIP | alleviating browser throttling on WebRTC transport READY | alleviating browser throttling on WebRTC transport May 31, 2018
@ya7ya
Copy link
Contributor Author

ya7ya commented May 31, 2018

@pgte Thanks for the pump package recommendation 👍 . it worked like a charm. no more race condition.

The tests are passing locally just fine. let me know there are any edits required 😄

@ya7ya
Copy link
Contributor Author

ya7ya commented May 31, 2018

BTW. the tests are passing successfully on my machine, I reckon it has to do with the wrtc and electron-wrtc build issues on linux mentioned here

testnodejs

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

LGTM. @ya7ya can you fix the commit messages?

@pgte can you give the final 👍

Copy link
Contributor

@pgte pgte left a comment

Choose a reason for hiding this comment

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

other than preventing the double callbacks, this looks good to me.

const block = new BlockStream(this.blockStreamSize, {nopad: true})

pump(channel, block, channel, (err) => {
if (err) return callback(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see a possible double-callback here when an error occurs. Maybe use the once package to make sure you only call the callback once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pgte , once is already used few lines before https://github.com/libp2p/js-libp2p-webrtc-star/pull/145/files/1b0d1ed130d6d447c3d0fc54b0abee13fdde11e7#diff-1fdf421c05c1140f6d71444ea2b27638R62

callback = callback ? once(callback) : noop
// ....
// ....
const block = new BlockStream(this.blockStreamSize, {nopad: true})

pump(channel, block, channel, (err) => {
  if (err) return callback(err)
})

Does that cover it or am i missing something here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha, it is, I missed it, sorry. Looks good to me!

const block = new BlockStream(this.blockStreamSize, {nopad: true})

pump(channel, block, channel, (err) => {
if (err) return callback(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm

@ya7ya ya7ya force-pushed the feat/throttling branch from 1b0d1ed to dbcab4d Compare June 1, 2018 12:40
@ya7ya
Copy link
Contributor Author

ya7ya commented Jun 1, 2018

I edited the commit msgs that weren't standard. so I had to push --force this , sorry about that 😅

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.

3 participants