Problem with inserting data to app's own container

@loziniak

@bochaco pointed these code examples out to me:

https://github.com/maidsafe/safe_examples/releases/download/0.13.0/examples.html

I tested them in my browser and it’s the first thing I’ve gotten to work on my mocknet. I suspect your problem is the precise order you’re making your API calls. The API gives everything piece-by-piece but documentation on how to put those pieces together is either lacking or I haven’t found it. These examples certainly fill in that gap, since they use the API calls, and they demonstrate the precise ordering of those calls.

1 Like

@drehb, I tried there and it also didn’t work. The sequence I used (without modifying any of the default code snippets):

safeApp: initialise
safeApp: authorise
safeApp: connectAuthorised
safeApp: getOwnContainer
safeMutableData: getEntries
safeMutableDataEntries: forEach
safeMutableDataEntries: insert
safeMutableDataEntries: forEach

Second “forEach” should give me something like “key1:value1”, but the output is the same as from the first “forEach”.

@BryanB, i’ve searched safe_examples on GitHub, and only place where “safeMutableDataEntries.insert” is found is WebApiPlayground’s code snippet. Which unfortunately gives me the same result.

Probably not helpful, but just in case, I’m using safeNfs.insert() (on the default container):

https://github.com/theWebalyst/remotestorage.js/blob/feature/safestore-backend-gd/src/safenetwork.js#L785

2 Likes

I noticed the markdown editor in safe_examples is also using the safeNfs calls.

I’m getting the impression safeNfs is the way to manage constant data, as opposed to ImmutableData.

Just in case anyone reading this thread doesn’t see this response from Josh on another thread:

2 Likes

But when you use safeMutableDataEntries.insert instead of mutations, you dont’t call safeMutableData.applyEntriesMutation, do you?

No, you don’t, you call window.safeMutableData.put instead.

So when you want to simply insert a key to a MD this way, you have to “getPermissions” also, just to “put” them back? Not very usable. Also, a “put” could be useful to have in documentation then: http://docs.maidsafe.net/beaker-plugin-safe-app/#windowsafemutabledataentriesinsert

Correct, and it might not even work as it may try to create the same MD again and thus failing. This is meant mainly for when you are creating the MutableData and setting it up with its initial set of permissions and data/entries rather than updating it which is when you should use mutations instead.

2 Likes

@loziniak I wouldn’t say “not very usable”, more this is how low-level APIs work, and we need to build higher level APIs over top this drudgery so that new developers to the project (like you and me) aren’t immediately overwhelmed by the number of calls necessary to do logical tasks like “update the key with this value”.

Being the change one wants to see, it’s up to us to make that higher level API.

EDIT: I’m really happy with the JavaScript class you’ve made to manage the stateful information that does not easily pass through Promises. However, what you’ve written is still quite low level. As I work on my project, I intend to build on top of your work thus far, and I will make sure to share progress via GitHub. For now, I’m still dotting all my Is and crossing all my Ts to get some simple examples of my own working. Apparently there’s one part of Promise chaining I still don’t completely understand (just when I thought I had it down). Once I work through that, I’ll likely be sending a PR or two your way.

8 Likes

@BryanB I’m only learning myself but happy to look at your Promises code if you think it might help.

@happybeing I appreciate that. I think my mistake is trying to nest promises within next(), it seems to go wrong when I do it. The code @loziniak made really helps linearise the promise chain, which is why I’m a fan. I already have example 1 from examples.html of safe_examples using the safe_api.js linked above, and I’m making steady progress on getting example 2 working.

Generally I understand Promises. I’ve worked with them a bit. There are just some subtle things I bump into, like nesting versus chaning. I like to nest Promises to work with external APIs, next()'ing their return values with my own to avoid global scoped variables. Unfortunately I cannot figure out how to combine nesting and chaining sensibly, so I’m just going with some stateful handle stores and linear Promise chain thanks to the safe_api.js above.

1 Like

@loziniak

Okay, so I tried to use your safe_api.js to reconstruct Example 2 from https://github.com/maidsafe/safe_examples/releases/download/0.13.0/examples.html

The biggest issue I ran into was Promise forking. Here’s what happened to me: My App Promise chain ran through from start to finish. Just before app.free(), I ran a function that creates a new MutableData, which forks this.handlePromise from App into this.handlePromise of the MutableData. Before the MutableData.then() contents could finish running, the App’s promise chain went right on into free() because they were not waiting on the MutableData chain. By the time the MutableData chain got around to checking on the data I pushed (strangely without error), the App was freed! Bing bang boom, it’s dead Jim.

So I made a couple modifications. App and App-descendants will have separate super classes. App-descendents will operate then() on this.parent.handlePromise, while App will operate then() on this.handlePromise. In this way, forks are avoided. I also tried to reduce some of the code complexity so that you can call this.then() instead of calling this.handlePromise = this.handlePromise.then().

I’ll submit an issue and PR on GitHub. I didn’t update all the other code to fit the new paradigm, I only updated what I needed. Hopefully it helps. There is still going to be a question of calling the right API bits in the right order, but man this Promise stuff has to be done just right.

It will need tweaking to get your example working, but it got my example working (as detailed in the Issue);

Hi. Great, thanks for a good job. Seems like the wrapper is starting to have live on it’s own :slight_smile: Maybe we should start a new thread on this project?

I have little time to dive into your code, but the forking issue seems a good one. Maybe freeing the app should wait for all forked promises to resolve? Or maybe we shouldn’t free the app at all, since it will be freed when the window is closed… Just a few alternative ideas to think about.

I was trying to keep function names and logic as close to original API as possible, to avoid confusion for people familiar with it. Perhaps instead of introducing new higher-level methods, we could stay at low-level and just add another abstraction as another wrapper? We could move things like authoriseAndConnect there, since it’s already a generalisation at higher level. What do you think?

Maybe freeing the app should wait for all forked promises to resolve?

Part of my additional code added free() for the App and fixed up free() for the MutableData. One issue with Promises is that a catch() is sort of a conditional branch that, once dealt with, will return to the following next(). Because of this, and other reasons, I don’t think we can rely on e.g. .then((mdHandle) => {}) to reliably provide the mdHandle unless it is directly the result of generating the mdHandle in the first place. So I’ve updated the code to store the App handle into App and the mdHandle into MutableData as stateful information. This is handy (I would argue necessary) for calling free() properly.

I’m not sure free() needs to be forced, it just needs to be supported. It works great in my PR. :wink:

Perhaps instead of introducing new higher-level methods, we could stay at low-level and just add another abstraction as another wrapper?

I’m not opposed. Effectively the low level wrapper would manage the Promise chain and keep stateful information like handles, and otherwise add very little other value. The higher level wrapper would operate over multiple low level calls to perform fairly atomic interactions, like saving a new key to an existing MutableData (which is a lot more than 1 low level call).

1 Like

Heya, I believe we absolutely need a wrapper as suggested. Are you guys working on one somewhere? Is this it?

Wouldn’t it make sense to abstract common patterns into recipes?

  1. Basic initialization in one function:
  1. Bootstrapping user, with a general auth function or more granular control with setupContainer and setupService:
  1. Reading and Updating mutable and immutable data

And so on, and so on. I haven’t gotten that deep into it yet. But it’s really making my head hurt trying to figure out how everything fits together for simple things like bootstrapping and service and reading and writing to the network.

Also I think code needs to be as clean and as readable as possible. Shouldn’t we be taking advantage that the browser has async/await native and write the wrapper using it’s clean syntax? The way we’ve started organizing our API utils in our project is staring to look pretty clean, check out this genNewAccount.js file. Let me know how I can help out with the wrapper :wink:

4 Likes

catch() is sort of a conditional branch that, once dealt with, will return to the following next()

But in this following next() only the second function for handling rejections will be fired, right? So the code relying on mdHandle shouldn’t be executed.

So catch() is one of those subtle things that can be confusing and it might not have been the best example to use.

First, let me say that if you do the following, A will print. The reason is that “catch” handles any exception and then switches back to the then track.

Promise.then(function () {throw 'OH NO';)
.catch(function (err) {console.log(err);})
.then(function () {console.log('A');});

Another note: once you have an exception, each then() in order is either skipped or it’s second argument is used: then(success, failure) is the same as then(success).catch(failure). You can have 15 then() and at the very end a single catch(). If the first then throws an exception, 14 thens are skipped, and the catch activates. For the longest time I thought you needed to catch on every then. What I’ve realized is that it’s best to catch only if you can fix the problem or you’re done executing.

Regardless of all that, if you don’t control the order of then calls, you cannot guarantee some then doesn’t get in between what returns mdHandle and what needs mdHandle. I think most people recognize this when they write their Promise chains relying on closure scoping to maintain their handle variable. The problem I have with that approach is that I find it difficult to avoid forking the Promise chain when nesting. If you’re already using class, and you’ve got some instance variables, why not use them? As soon as your App or MutableData or MutableDataEntry has a handle, the handle should get stored to that object. No need for closure nesting, no need to rely on the previous thenable returning your handle. It’s just there, in your object, ready to go. You also get the benefit of knowing exactly what to free() in each class.

1 Like