Probably Promise overuse?

loziniak · August 19, 2017, 3:50pm

Don’t you get an impression, that returning Permissions from API calls is not always justified? Aren’t there functions that do not require network calls and are just IPC calls between main and renderer processes that are immediate because they happen within the same machine? For example safeMutableDataPermissionsSet.setAllow or safeCrypto.generateEncKeyPair. Do they call the network? I think there should be as many synchronous calls as possible. They are much more simple to use and do not leave us with “then” on every code line.

As to “thens”:

https://github.com/maidsafe/safe_examples/blob/master/email_app/app/safenet_comm.js#L221

… do you really have to create a function object for a simple assignment operation, inboxMd = md etc.?

bzee · August 20, 2017, 3:50pm

I share your opinion, there seem to be some calls that seemingly have little reason to be asynchronous, like CryptoInterface.sha3Hash. It probably has something to do with how the SAFE client library is called using a Foreign Function Interface (FFI).

By the way, you can rewrite the then chain to make use of async and await:

(async () => {
    let md      = await app.mutableData.newRandomPublic(CONSTANTS.TAG_TYPE_INBOX)
    let inboxMd = await md.quickSetup(baseInbox)
    let permSet = await app.mutableData.newPermissionSet()
    await permSet.setAllow('Insert')
    await inboxMd.setUserPermissions(null, permSet, 1)
})()

Should do the exact same thing – didn’t test.

Mindphreaker · August 20, 2017, 8:36pm

This sounds fantastic but unfortunately it doesn’t seem to work in Beaker. It throws a syntax error.

Mindphreaker · August 21, 2017, 7:36am

@bzee just try to declare a function with async:

async function test() {};

Throws the following error when page loads:

Uncaught SyntaxError: Unexpected token function

bzee · August 21, 2017, 8:20am

Interesting, is that the whole snippet? Where are you executing that JavaScript? Beaker has been executing it fine for me.

If you’re targeting an older system, then I recommend using a transpiler (e.g. Babel).

Mindphreaker · August 21, 2017, 9:13am

Well, I don’t even execute it. The error occures right after the page loads. My whole html is a script tag containing the above js line.

Which version of SAFE browser do you use? Mine is 0.4.0

What do you mean by this? I use Win 10 with SAFE browser (compiled by maidsafe) and a simple html file.
EDIT: In chrome this works without problems. @joshuef any thoughts?

bzee · August 21, 2017, 9:55am

I know it’s ES2017, and therefore fairly experimental, but modern browsers should be handling it fine. I find the error message that you get a little weird, as you’d expect it to complain about the async token, not about function. I’ve been running it on a slightly more recent version (7dbcf86).

I can’t get 0.4.0 to work on a Windows 10 environment to replicate your issue (using npm install):

> safe-app@0.1.0-dev.1 postinstall C:\Users\benno\dev\src\safe_browser-0.4.0\app\node_modules\safe-app
> download_deps --package package.json

Downloading tmp-9208-0-system_uri-1.0.0-pre-win-x64.zip
Error: GET https://github.com/gnunicorn/system_uri/releases/download/v/system_uri-1.0.0-pre-win-x64.zip returned 404

Notice the wrong URL (/v/ part). The build process goes fine until the download_deps command is issued.

loziniak · August 21, 2017, 6:32pm

This is probably the reason for overuse of async api:

github.com

maidsafe/rfcs/blob/master/text/0043-async-safe-core/0043-async-safe-core.md

# Async safe_core
- Status: implemented
- Type: feature
- Related components: `safe_core`, `safe_launcher`
- Start Date: 02-September-2016
- Discussion: https://forum.safedev.org/t/rfc-43-async-safe-core/99

## Summary
Making `safe_core` async with respect to FFI and internally.

## Conventions
- The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in [RFC 2119](http://tools.ietf.org/html/rfc2119).

## Motivation
Currently `safe_core` has a threaded design. It has properly mutexed core-objects and parallel invocations can be achieved by making calls from different threads. However the frontend coded in NodeJS interfaces via `NodeFFI` which detects the hardware concurrency and limits the number of threads to that. This is not bad but leads to underutilisation of `safe_core`. This RFC proposes a design in which number of invocations are not limited. While this will lead to optimal usage of `safe_core` itself, it can cause very high network traffic, so we will try and address such problems too.

## Detailed design
Let's assume the hardware concurrency is 2 (dual core) for a machine. In current threaded design, if `App_0` makes 2 requests to `Launcher`, `App_1` makes 2, `NodeFFI` module in `Launcher` queues 2 of them and sends only 2 to `safe_core` spawing 2 threads for parallelism. Until atleast one thread returns, none of the 2 queued requests will have any chance of getting through to `safe_core`. `safe_core` invariably waits tremendous amount of time (in terms of CPU speeds) for network responses (_IO_). While it sits doing nothing it could have handled so many more requests if only `NodeFFI` forwarded it. This leads to underutilisation of the core library. However there are positives to this. For instance if `NodeFFI` were to spawn a thread per request, then we could take that to the other extreme saying that 30 combined requests from a few apps will result in 30 threads which is also not good. Further, by restricting number of simultaneous invocations the throttling mechanism can be view as in-built. One would not be allowed to choke the network/bandwidth by making 100's of concurrent requests. So we need to strike a balance.

To better utilise the library without uncontrolled spawning of resource hogging threads, this RFC proposes async design involving single threaded event loop. We will use `futures-rs` crate. There will be a central event loop running on a single thread which registers futures and dispatches them when ready.

This file has been truncated. show original

Krishna · August 22, 2017, 1:13pm

Async await is a cool feature. It is supported from 7.10.0 nodejs or above I guess. The browser previously was compatible only with 6.9 of nodejs. This will be supported once we update to the nodejs version from which async/await can be supported.

Edit: my comment is related to the usage of async await in DOM layer

bzee · August 22, 2017, 2:45pm

@Krishna, are you sure the NodeJS version influences the support of the Beaker browser? I have been using async/await just fine, even though I compiled the SAFE browser with 6.5.0:

Krishna · August 22, 2017, 3:47pm

Have not tried it on DOM. I said that based on the feature list from node.green. When I used to do some work on browser code, it never compiled with async/await. I will check it out when I get sometime @bzee.

Mindphreaker · August 23, 2017, 11:32am

I’ve opened an issue so you can test / track this when building for Windows:

hunterlester · August 23, 2017, 1:54pm

Running Windows 10 and a build of safe_browser master branch.

Async is working fine in the console.

I’ll try it on, https://github.com/maidsafe/safe_browser/releases/download/0.4.0/safe-browser-v0.4.0-win-x64.zip, as well

joshuef · August 24, 2017, 8:05am

@Mindphreaker, @bzee, @hunterlester the core electron dependency was updated in a version later than 0.4.0, which I think @bzee is using, which I suspect brings the async functionality. Prior to this we were on a really old version of electron.

Mindphreaker · August 24, 2017, 9:54am

Sounds great, looking forward to the upcoming release!