search

orbitdb-archive / orbit-db-pubsub

Message propagation module for orbit-db
MIT License
19 stars 14 forks source link

fix: make unsubscribe/disconnect async #12

Closed mistakia closed 6 years ago

mistakia commented 6 years ago

Update IPFSPubsub.unsubscribe and IPFSPubsub.disconnect to be async as js-ipfs-api has updated pubsub.unsubscribe to be async.

IPFSPubsub.unsubscribe and IPFSPubsub.disconnect can be used with callbacks or if no callback is provided it will return a promise so that you can use async/await.

I also updated IPFSPubsub.subscribe to receive an optional callback - otherwise it will throw an error, like before.

potentially related: orbitdb/orbit-db#370

shamb0t commented 6 years ago

Thank you for this PR @mistakia! :heart: Just one thing: I see you've done a lot of work to accommodate a new callback parameter, however we don't use return callbacks anywhere in the OrbitDB code base and aim to keep the code promise/await-based. Sorry this is not yet explicit, we're working on a standard to make it clear in the contribution guidelines! Care to amend your PR to remove the additional callback parameter?

mistakia commented 6 years ago

At the time I was thinking this module might be depended on outside of OrbitDB 🤔😂

5b067aa991cedc3250d79bf796810718804e0a51 LGTM (on orbitdb/orbit-db-pubsub#tests/fix-race)

haadcode commented 6 years ago

@mistakia would you want to pull that ^ branch into your branch and to this PR? You've done amazing job on finding the cause for the race condition (and they always take time to figure out) and did a lot of work to fix it (thank you!) and wouldn't want that work go waste.

Would love to have this as your contribution to the repo! 🙏

mistakia commented 6 years ago

I'm just glad to help - the nice thing about helping while not having commits merged is you can't get git-blame'd.

I've merged your branch here. Let me know if there's something else I can do here.

haadcode commented 6 years ago

I'm just glad to help - the nice thing about helping while not having commits merged is you can't get git-blame'd.

We're extremely grateful for your help! You can git-blame it on me ;)

haadcode commented 6 years ago

Before merging, let's make sure this code actually fixes the race condition. I believe we only need to run the tests again (several times as you did earlier) in https://github.com/orbitdb/orbit-db/pull/408, right?

mistakia commented 6 years ago

Given the way I have the dep setup in package.json, I'm assuming CircleCI should pull in the latest commits?

I've run it a few times here. So far so good, except there was one failure for what seems to be a db not being able to be opened for a LOCK being held. I'll run it a few more times to make sure.

haadcode commented 6 years ago

If you get fails in CI, perhaps try "Rebuild without cache" (to make sure Circle pulls in deps afresh).

mistakia commented 6 years ago

Ready for another review.

I needed to use "Rebuild without cache" to pull in the latest commits. It failed due to an unhandled promise rejection because pseries expects an array of promise-returning functions (instead of the promises themselves).

I'm not very familiar with pseries so let me know if there's a more elegant solution. Otherwise, tests look good.

haadcode commented 6 years ago

@mistakia LGTM! 👍 Will run the tests a few more times just to make sure https://github.com/orbitdb/orbit-db/pull/408 passes with this PR.

If all good, I'll go ahead and merge, publish the new module and get ^ PR in orbitdb merged as well.

haadcode commented 6 years ago

@mistakia thank you for digging into this, finding the reason for the bug and doing all the work to fix it. Appreciate it tremendously! ❤️Great job! 👍

Will do the dance of pushing this uptream and get it merged to orbitdb.

haadcode commented 6 years ago

The updates you made to the README got lost during the process. I've added them back in here https://github.com/orbitdb/orbit-db-pubsub/commit/0889162776877b94422478726ec06ec7cb0130fd.