Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: mfs implementation #1360

Merged
merged 2 commits into from
Jul 5, 2018
Merged

feat: mfs implementation #1360

merged 2 commits into from
Jul 5, 2018

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented May 18, 2018

Not ready for merging yet but I'd love some feedback on the approach. I've placed the mfs commands behind a flag named --enable-mfs-experiment - if you enable this it will cause the ipfs files subcommands to be the same as the go implementation.

@ghost ghost assigned achingbrain May 18, 2018
@ghost ghost added the status/in-progress In progress label May 18, 2018
@Mr0grog
Copy link
Contributor

Mr0grog commented May 18, 2018

Minor note — would you please document the new EXPERIMENTAL flag along with the others in the README?

@alanshaw alanshaw changed the title feat: mfs implementation [WIP] feat: mfs implementation May 21, 2018
@achingbrain achingbrain force-pushed the add-mfs branch 2 times, most recently from a0fd5b9 to 259aa82 Compare May 26, 2018 07:07
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

You're right, we should really be reading config from an existing config file and overriding with --enable-XXX-experiment or env vars when set.

I'm ok with loading MFS functions up like this from a separate module. Was your plan to leave it like this or eventually merge the ipfs-mfs repo with this one when it's all done?

src/cli/bin.js Outdated

return commandObject
}
})
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind explaining what's going on here?

Copy link
Member Author

@achingbrain achingbrain May 29, 2018

Choose a reason for hiding this comment

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

It's preventing any of the existing ipfs files <command> commands from loading if enableMfs is truthy.


describe('file ls', () => runOnAndOff((thing) => {
let ipfs

before(function () {
this.timeout(50 * 1000)
ipfs = thing.ipfs
return ipfs('files add -r test/fixtures/test-data/recursive-get-dir')
return ipfs('add -r test/fixtures/test-data/recursive-get-dir')
Copy link
Member

Choose a reason for hiding this comment

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

So is this suggesting that add, get and cat would no longer be available under the files namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is consistent with go-ipfs

Copy link
Member

Choose a reason for hiding this comment

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

Let's think more about this...

I would be totally down to move forward with ipfs/specs#98 in JS land (both js-ipfs and js-ipfs-api). In fact, I believe we are reaching a time where we need to start pushing forward better and polished APIs, given that we live in a different ecosystem and we need to make things that work for our user needs.

An advantage to ourselves is that we have a SPEC and tests, making this transition smooth and more pleasant.

@achingbrain
Copy link
Member Author

Was your plan to leave it like this or eventually merge the ipfs-mfs repo with this one

@diasdavid's preference was for a separate module so there are no plans to merge it into this one AFAIK.

@daviddias
Copy link
Member

@achingbrain can we drop the experimental flag? This should work with go-ipfs right off the bat.

@achingbrain
Copy link
Member Author

achingbrain commented Jun 1, 2018 via email

achingbrain added a commit to ipfs/js-ipfsd-ctl that referenced this pull request Jun 4, 2018
We are going all-in with mfs and not having it behind a flag: ipfs/js-ipfs#1360 (comment)
vasco-santos pushed a commit to ipfs/js-ipfsd-ctl that referenced this pull request Jun 6, 2018
We are going all-in with mfs and not having it behind a flag: ipfs/js-ipfs#1360 (comment)
@achingbrain achingbrain force-pushed the add-mfs branch 11 times, most recently from 2e73d97 to 4fed9c0 Compare June 14, 2018 12:48
@achingbrain achingbrain force-pushed the add-mfs branch 4 times, most recently from 8c6a548 to 4406bb7 Compare June 20, 2018 09:18
@achingbrain achingbrain force-pushed the add-mfs branch 4 times, most recently from aec7c8a to a1d2a01 Compare July 3, 2018 14:11
@lidel
Copy link
Member

lidel commented Jul 3, 2018

May be too specific, but is there an interop test that checks if files.write produces the same CIDs in js-ipfs and go-ipfs?
We will be changing default CID encoding at some point and such tests would help us coordinate.

@achingbrain achingbrain force-pushed the add-mfs branch 2 times, most recently from 94228dd to bd4d34b Compare July 4, 2018 13:29
@achingbrain
Copy link
Member Author

@diasdavid @lidel the interop tests are here: ipfs/interop#25 and there are a whole bunch that assert on CIDs generated by go & js.

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

This is incredible, thanks for your patients @achingbrain

} else if (arg.error && arg.error.message) {
print(arg.error.message)
} else {
print('Unknown error, please re-run the command with DEBUG=ipfs:cli to see debug output')
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to have some consistency here - the rest of the cli debug prefixes are cli:* and elsewhere in the code we have jsipfs:*. One for a separate PR 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd love it if we could standardise on one prefix for all ipfs related DEBUG values. Just doing ipfs:* instead of ipfs:*,ipfsd-ctl:*,ipfs-api:*,etc is my dream.

src/cli/bin.js Outdated
print('Unknown error, please re-run the command with DEBUG=ipfs:cli to see debug output')
}

process.exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

We always called cleanup before even if there was an error. I think this needs to be added here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably register a process exit/error handler to do the cleanup instead, but we'd need a synchronous way of closing the repo which I don't think exists yet.

@@ -41,7 +41,7 @@ module.exports = {
// * reads as 'agree in'
if (node._json) {
delete node._json.multihash
node._json.data = '0x' + node._json.data.toString('hex')
node._json.data = node._json.data.toString('base64')
Copy link
Member

Choose a reason for hiding this comment

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

Is this change meant to be in this PR? Why not also for the if test below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably could be separate. go prints out the data as base64 encoded, we print it out as hex.

const mfs = components.mfs(this)

Object.keys(mfs).forEach(key => {
if (mfs.hasOwnProperty(key)) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: I believe Object.keys iterates over own properties so I think this check is redundant, unless I'm missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

From my understanding Object.keys returns all properties, including inherited ones whereas hasOwnProperty will only return true if it's a non-inherited property. Happy to change this if you prefer it the other way.

test/browser.js Outdated
if (isWebWorker) {
// https://github.com/Joris-van-der-Wel/karma-mocha-webworker/issues/4
global.MFS_DISABLE_CONCURRENCY = true
}
Copy link
Member

Choose a reason for hiding this comment

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

Since there is a requirement to either set this variable or do some setup on the main thread (for webworkers) we should mention it and link to it from the js-ipfs README so that it's easily discoverable and we don't get issues opened about it here.

Also a separate PR to add a examples for using MFS in webworkers with/without concurrency is needed and would be very 🚀 🎸 ✨ 💖

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a paragraph to the README, will create an example in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

What does MFS with concurrency mean?

Copy link
Member

Choose a reason for hiding this comment

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

nvm. Read the paragraph.


tests.key(CommonFactory.create({
spawnOptions: {
args: ['--pass ipfs-is-awesome-software'],
initOptions: { bits: 512 }
initOptions: { bits: 1024 }
Copy link
Member

Choose a reason for hiding this comment

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

This is for faster boot - defaultCommonFactory uses 512 also, is this change needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I ran the tests against the go daemon and it refused to start up with 512 and said it should be 1024 as a minimum. Can change it back if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Yep in js-ipfs-api it's set to 1024 it tests against go-ipfs, but these tests only run against js-ipfs and there's no restriction in place currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

There probably should be, for the sake of interop though, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes good point, lets not slow down the tests unnecessarily until that PR comes in though. I'll create an issue for adding it so we can track it.

README.md Outdated
@@ -135,6 +136,20 @@ You can also load it using a `<script>` using the [unpkg](https://unpkg.com) CDN
```
Inserting one of the above lines will make an `Ipfs` object available in the global namespace.

### Use with Web Workers

The `ipfs.files.*` [MFS](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md#mutable-file-system) commands use LevelDB to store the CID that corresponds to the current root of the file system. LevelDB has no concurrency control so a read/write lock at the application level is necessary. With single-process and [Node.js cluster](https://nodejs.org/api/cluster.html) based apps this is completely transparent but [Web Workers](https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API) require a little extra setup. Use the [`observable-webworkers`](https://www.npmjs.com/package/observable-webworkers) module to allow communication with any Web Workers your application creates:
Copy link
Member

Choose a reason for hiding this comment

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

I see. This is so that if multiple WebWorkers are spawn, the store is shared?

Note that this is valid for anything that IPFS stores really, in the browser it is IndexedDB for everything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, if multiple threads mutate the MFS graph at the same time they'll result in different hashes for the root of the graph creating a last-update-wins race condition. There needs to be some sort of gatekeeper to resolve this.

Copy link
Member

Choose a reason for hiding this comment

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

Multiple threads, each with its IPFS Node or just one IPFS node?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've flipped this round so that you only need to do the config dance if you've spawned a node on the main process then forked (e.g. node clusters) or spawned nodes on the UI thread and web workers (e.g. browsers) all using the same repo - which hopefully shouldn't be very common.

I'm not sure we should encourage this in the browser at least so I've left it undocumented for the time being but we can discuss this during the locking session at the dev meetup next week.

@ghost ghost assigned alanshaw Jul 5, 2018
@alanshaw alanshaw merged commit 871d24e into master Jul 5, 2018
@ghost ghost removed the status/in-progress In progress label Jul 5, 2018
@alanshaw alanshaw deleted the add-mfs branch July 5, 2018 21:43
@achingbrain
Copy link
Member Author

no-wai

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.

5 participants