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

refactor: use new async await APIs and update ipfsd-ctl #96

Merged
merged 24 commits into from
Feb 12, 2020

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Jan 22, 2020

resolves #92 - does not specify bits anymore
resolves #31 - uses "test" profile for all daemon spawns
resolves #30 - new ipfsd-ctl exposes daemon logs via DEBUG=ipfsd-ctl:daemon:std*

TODO:

  1. fix the websocket-star tests - websocket-star was not converted to async/await and has a security vulnerability. Switch to using stardust or webrtc-star for the tests
  2. fix ipfsd-ctl to auto kill process if it does not stop within a timeout. The previous version of ipfsd-ctl had this feature and was killing go-ipfs daemons. I've observed here and in master that go-ipfs does not quit when running tests where it sends files to/from a js-ipfs node. @Stebalien were you aware?
  1. fix issue with ky never returning from a call to res.json() if the payload is BIG. IDK why but ky clones the response, which causes it to be piped to two places, the first is never consumed so for large responses the high water mark is hit and the stream stops flowing. It's similar to the problem we worked around here.

TODO:

1. [ ] fix the websocket-star tests - websocket-star was not conevrted to async/await and has a security vulnerability. Switch to using stardust or webrtc-star for the tests
1. [ ] fix ipfsd-ctl to auto kill process if it does not stop within a timeout. The previous version of ipfsd-ctl had this feature and was killing go-ipfs daemons. I've observed here and in master that go-ipfs does not quit when running tests where it sends files to/from a js-ipfs node. @Stebalien were you aware?
1. [ ] fix issue with ky never returning from a call to `res.json()` if the payload is BIG. IDK why but ky clones the response, which causes it to be piped to two places, the first is never consumed so for large responses the high water mark is hit and th stream stops flowing. It's similar to the problem we [worked around here](https://github.com/ipfs/js-ipfs-http-client/blob/d7eb0e8ffb15e207a8a6062e292a3b5babf35a9e/src/lib/error-handler.js#L12-L23).
@jacobheun
Copy link
Contributor

fix the websocket-star tests - websocket-star was not converted to async/await and has a security vulnerability. Switch to using stardust or webrtc-star for the tests

I'll work on converting this over to webrtc-star

@Stebalien Stebalien removed their request for review January 22, 2020 13:49
@Stebalien
Copy link
Member

I've observed here and in master that go-ipfs does not quit when running tests where it sends files to/from a js-ipfs node. @Stebalien were you aware?

Go-ipfs will try to wait for all in-progress commands to stop. If that doesn't work, it'll die after 10s.

We try to cancel in-progress commands but could be getting stuck. Could you file an issue somewhere? We should try killing go-ipfs with SIGQUIT sometime to see where it's getting stuck (that will dump a stack trace).

@hugomrdias
Copy link
Member

@Stebalien should we wait for the 10s or just SIGKILL please have a look at ipfs/js-ipfsd-ctl#441 (review)

jacobheun and others added 13 commits January 24, 2020 15:20
* fix: throw if no requested address is available

We loop through available multiaddrs looking for certain types of
address in our tests.  If there are non of the type we are looking
for, we don't return anything which results in undescriptive
error messages from libp2p.

The change here is to throw an error with an explicit message if,
for example, no webrtc-star address is available when we expect
there to be.

* chore: pr comments

Co-Authored-By: Alan Shaw <alan.shaw@protocol.ai>
@achingbrain
Copy link
Member

achingbrain commented Feb 5, 2020

Looks like it just needs the two gh urls removing from package.json and it's good to go.

@alanshaw build is green - is the fix for the ky issue mentioned at the top still required?

package.json Outdated Show resolved Hide resolved
@alanshaw
Copy link
Member Author

alanshaw commented Feb 6, 2020

@alanshaw build is green - is the fix for the ky issue mentioned at the top still required?

The test is skipped. It still needs fixing (i.e. remove ky from ipfs-http-client) but we can probably merge this, open an issue and revisit when we've fixed the ky issue.

cc @hugomrdias

@hugomrdias
Copy link
Member

@alanshaw build is green - is the fix for the ky issue mentioned at the top still required?

The test is skipped. It still needs fixing (i.e. remove ky from ipfs-http-client) but we can probably merge this, open an issue and revisit when we've fixed the ky issue.

cc @hugomrdias

yes, ill remove ky asap and come back to this issue.

@achingbrain
Copy link
Member

I'm going to merge this since this branch is basically defacto master right now.

I've opened #101 to track the skipped test.

@achingbrain achingbrain merged commit a992ea1 into master Feb 12, 2020
@achingbrain achingbrain deleted the refactor/async-await branch February 12, 2020 16:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants