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.

Sign up for GitHub

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

Jump to bottom

feat: set and hook up libp2p-connection-manager #184

Merged
merged 6 commits into from
Jun 20, 2018
Merged

feat: set and hook up libp2p-connection-manager #184

merged 6 commits into from
Jun 20, 2018

Conversation

pgte
Copy link
Contributor

@pgte pgte commented Apr 6, 2018

Solves #183

@ghost ghost assigned pgte Apr 6, 2018
@ghost ghost added the status/in-progress In progress label Apr 6, 2018
@pgte pgte requested a review from daviddias April 6, 2018 09:48
@pgte
Copy link
Contributor Author

pgte commented Apr 6, 2018

@diasdavid I'm not sure about using options.connectionManager. Since you're more familiar with the libp2p API, what do you think?

@victorb
Copy link
Member

victorb commented Apr 6, 2018

error Couldn't find package "libp2p-connection-manager" on the "npm" registry. errors all over CI

@pgte
Copy link
Contributor Author

pgte commented Apr 6, 2018

@victorbjelkholm that's expected, it's not released yet and I can't do that. @diasdavid ?

@pgte
Copy link
Contributor Author

pgte commented Apr 13, 2018

@diasdavid ping

@daviddias
Copy link
Member

@pgte just released it for you and added you as an owner. I'm intrigued on why you weren't able to do it? It was the first time the module got published -- https://www.npmjs.com/package/libp2p-connection-manager.

@pgte
Copy link
Contributor Author

pgte commented Apr 23, 2018

@diasdavid I never did ipfs or libp2p releases, it would have been the first time, so I would have needed the go-ahead. I've seen you added me as an npm owner. Thank you!

@pgte pgte closed this Apr 23, 2018
@ghost ghost removed the status/in-progress In progress label Apr 23, 2018
@pgte pgte reopened this Apr 23, 2018
@ghost ghost added the status/in-progress In progress label Apr 23, 2018
@pgte pgte requested a review from hugomrdias April 23, 2018 08:53
@daviddias
Copy link
Member

CI is still red. Any ETA to get this ready for prime time?

@daviddias daviddias mentioned this pull request Apr 30, 2018
Closed
30 tasks
@pgte
Copy link
Contributor Author

pgte commented Apr 30, 2018

Updated the libp2p-connection-manager dev version, but the CI is still breaking, now with the errors:

TravisCI:

Error: Unable to find native module
    at requireNativeModule (/home/travis/build/libp2p/js-libp2p/node_modules/node-cmake/index.js:40:20)
    at Object.<anonymous> (/home/travis/build/libp2p/js-libp2p/node_modules/wrtc/lib/binding.js:3:39)
    at Module._compile (internal/modules/cjs/loader.js:678:30)

Jenkins:

  1) stream muxing spdy + mplex:
     Uncaught Error: Unhandled "error" event. (undefined)
      at node_modules/spdy-transport/lib/spdy-transport/connection.js:757:12
      at Array.forEach (<anonymous>)
      at Connection.destroyStreams (node_modules/spdy-transport/lib/spdy-transport/connection.js:753:33)

and others...

I don't think they're related to these changes. @victorbjelkholm @diasdavid can you help?

@victorb
Copy link
Member

victorb commented Apr 30, 2018

@pgte I rerun the tests and now they are passing. I would add a comment above the test-case, noting that it's flaky.

@ghost ghost assigned daviddias May 6, 2018
@daviddias
Copy link
Member

@pgte could you describe what happens with the connection manager enabled?

@pgte
Copy link
Contributor Author

pgte commented May 7, 2018

@diasdavid since the defaults impose no threshold, nothing should happen.

@alanshaw alanshaw mentioned this pull request Jun 1, 2018
Closed
23 tasks
@alanshaw
Copy link
Member

alanshaw commented Jun 1, 2018

This is awesome! How would someone pass connection manager options to js-ipfs to enable the limits? Do we need a PR to js-ipfs to allow it?

We should also document the options in js-ipfs (or at least link to the relevant section on the connection manager repo).

Also, is there something left to do on this PR?

@pgte
Copy link
Contributor Author

pgte commented Jun 1, 2018

I think this is working, but I'd like to get some consensus around how the connection manager options are passed in. Right now, it's from options.connectionManager. Sounds good?

@alanshaw
Copy link
Member

alanshaw commented Jun 1, 2018

Looks sensible to me

@pgte
Copy link
Contributor Author

pgte commented Jun 1, 2018

Awesome. Then this should be all! :)

@alanshaw
Copy link
Member

alanshaw commented Jun 8, 2018

@diasdavid can we get this merged?

@daviddias
Copy link
Member

CI is red. Can we get it green?

@alanshaw
chore: upgrade to latest libp2p-switch
f915c25 
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
@ghost ghost assigned alanshaw Jun 20, 2018
@alanshaw
Copy link
Member

Ok so, I've fixed the merge conflict, which brings in updated dependencies. We're now getting the exact same test failures in CI that I'm seeing in master (I've logged them here also) so I think they might be unrelated to the changes in this PR.

@alanshaw alanshaw changed the title set and hook up libp2p-connection-manager feat: set and hook up libp2p-connection-manager Jun 20, 2018
daviddias
daviddias reviewed Jun 20, 2018
View reviewed changes
package.json Outdated
"libp2p-websocket-star": "~0.8.0",
"libp2p-websocket-star-rendezvous": "~0.2.3",
"lodash.times": "^4.3.2",
"pull-goodbye": "0.0.2",
"pull-serializer": "~0.3.2",
"pull-stream": "^3.6.8",
"sinon": "^5.0.7",
"libp2p-webrtc-star": "~0.15.0",
"wrtc": "0.1.1"
"wrtc": "~0.1.4"
Copy link
Member

Choose a reason for hiding this comment

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

@alanshaw don't upgrade this until #194 is solved.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a bug report open on the wrtc repo?

@alanshaw
fix: pin wrtc until #194 is resolved
97a9bbf 
#194

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
@alanshaw
Copy link
Member

alanshaw commented Jun 20, 2018 •
edited
Loading

aha, tests have now passed on Node.js 8 but fail on Node.js 10 because there's no prebuilt binary for wrtc. I think we good to merge? 💣 💥

@daviddias daviddias merged commit d597204 into master Jun 20, 2018
@daviddias daviddias deleted the feat/connection-manager branch June 20, 2018 10:19
@ghost ghost removed the status/in-progress In progress label Jun 20, 2018
@daviddias daviddias mentioned this pull request Jul 5, 2018
Closed
3 tasks
maschad pushed a commit to maschad/js-libp2p that referenced this pull request Jun 21, 2023
@mpetrunic
fix: cannot catch EADDRINUSE (libp2p#198)
c7312db 
related to libp2p#184
maschad pushed a commit to maschad/js-libp2p that referenced this pull request Jun 21, 2023
@semantic-release-bot
chore(release): 5.0.1 [skip ci]
eb6e2cd 
## [5.0.1](libp2p/js-libp2p-websockets@v5.0.0...v5.0.1) (2022-12-08)

### Bug Fixes

* cannot catch EADDRINUSE ([libp2p#198](libp2p/js-libp2p-websockets#198)) ([c7312db](libp2p/js-libp2p-websockets@c7312db)), closes [libp2p#184](libp2p/js-libp2p-websockets#184)

### Dependencies

* **dev:** bump @libp2p/interface-mocks from 7.1.0 to 8.0.2 ([libp2p#199](libp2p/js-libp2p-websockets#199)) ([daff533](libp2p/js-libp2p-websockets@daff533)), closes [libp2p#318](https://github.com/libp2p/js-libp2p-websockets/issues/318) [libp2p#315](https://github.com/libp2p/js-libp2p-websockets/issues/315) [libp2p#313](https://github.com/libp2p/js-libp2p-websockets/issues/313) [libp2p#312](https://github.com/libp2p/js-libp2p-websockets/issues/312)
* **dev:** bump it-all from 1.0.6 to 2.0.0 ([libp2p#193](libp2p/js-libp2p-websockets#193)) ([6213f8f](libp2p/js-libp2p-websockets@6213f8f)), closes [libp2p#28](libp2p/js-libp2p-websockets#28) [libp2p#28](libp2p/js-libp2p-websockets#28) [libp2p#27](libp2p/js-libp2p-websockets#27) [libp2p#24](libp2p/js-libp2p-websockets#24)
* **dev:** bump it-drain from 1.0.5 to 2.0.0 ([libp2p#191](libp2p/js-libp2p-websockets#191)) ([e549691](libp2p/js-libp2p-websockets@e549691)), closes [libp2p#28](libp2p/js-libp2p-websockets#28) [libp2p#28](libp2p/js-libp2p-websockets#28) [libp2p#27](libp2p/js-libp2p-websockets#27) [libp2p#24](libp2p/js-libp2p-websockets#24)
* **dev:** bump it-take from 1.0.2 to 2.0.0 ([libp2p#192](libp2p/js-libp2p-websockets#192)) ([4c037fc](libp2p/js-libp2p-websockets@4c037fc)), closes [libp2p#28](libp2p/js-libp2p-websockets#28)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanshaw alanshaw alanshaw left review comments

@daviddias daviddias daviddias approved these changes

@hugomrdias hugomrdias Awaiting requested review from hugomrdias

Assignees

@pgte pgte

@alanshaw alanshaw

@daviddias daviddias

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pgte @victorb @daviddias @alanshaw