Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save the project, by figuring out how to make it usable. #105

Closed
jedisct1 opened this issue Sep 28, 2017 · 35 comments
Closed

Save the project, by figuring out how to make it usable. #105

jedisct1 opened this issue Sep 28, 2017 · 35 comments

Comments

@jedisct1
Copy link
Owner

jedisct1 commented Sep 28, 2017

Here's a description of the current state of affairs.

libsodium, the core library, works amazingly well after having been compiled to asm.js and to webassembly, thanks to Emscripten.

All the tests are passing, speed isn't too bad, it's great!

However, using Emscripten-compiled code from regular Javascript code requires additional code before and after every call, in order to play well with its conventions and the way memory is managed.

libsodium-wrappers solves this, by thinly wrapping the Emscripten-compiled function calls, to provide something easier to use, more idiomatic.

libsodium-wrappers is designed to be a module. Something one should be able to use using require(), amd, ES6 modules. And, for grey beards, one should include a script in a web page, and get something in the global namespace (currently named sodium).

That thing naturally depends on libsodium, the core library. So, including libsodium-wrappers as a module should include libsodium as a module, and everything will work fine.

Unfortunately, the addition of webassembly messes things up a little bit.

libsodium compiled to webassembly works well. It's fast. It's awesome. It's the future.

So, we want libsodium-wrappers to load either the asmjs version, or the webassembly version of libsodium, according to what the runtime supports.

libsodium-wrappers could just require('libsodium.js') or require('libsodium-wasm.js'). That's what was attempted.

But libsodium-wasm.js is just a loader, generated by emscripten, that loads the actual wasm binary code (libsodium.wasm). That, obviously, has to happen asynchronously.

So, we can't require('libsodium-wasm.js') and expect symbols to be usable right away. We need an asynchronous interface. Either by returning a Promise, or by forcing applications to provide a callback.

What applications must be able to do in order to use that project, and what they may expect:

  • require('libsodium-wrappers.js') or equivalent
  • or <script src=libsodium-wrappers.js></script>
  • And either a callback is called when everything's ready, or the require() returns a promise that resolves to the list of exported symbols once everything is loaded.
  • If wasm is used, it has to be compiled, possibly in a worker because some browsers require it, with a proxy for the provided functions in the main thread. Emscripten's Webassembly support seems to include something to automatically generate this.

And that's it.

It's probably straightforward to people who have some good experience with Javascript.

Unfortunately, I haven't been able to achieve this. I spent a significant amount time on this, and miserably failed.

I give up.

I'd be glad to keep maintaining the Javascript/Webassembly version of the core library. But I can't do the plumbing alone.

I can't afford to spend more time making both work together and packaged in a way that applications can easily use.

If you can help, please do. Maybe it's as simple as an option to enable in Emscripten, or some tiny Javascript glue to add somewhere. I don't know.

If no one can help, I will delete this project and refocus on things I am more comfortable with.

@jedisct1
Copy link
Owner Author

The dist directory now contains unoptimized, unpacked files.

These are not designed to be used in any application. They can be useful to understand what the current code looks like, to experiment with it, and possibly find a solution to the above problem.

@buu700
Copy link
Contributor

buu700 commented Sep 28, 2017

I've been planning on submitting a PR for exactly this at some point in the next few months based on the ideas I mentioned in #94. It's been working great in the libraries I listed there; we'd probably just want to wait for emscripten-core/emscripten#5296 to be merged before doing the same thing here.

This would solve the issue you noted of requiring an external binary to be loaded at run-time, but doesn't help with the async aspect since wasm compilation still generally needs to be asynchronous. A few ideas come to mind as solutions for that:

  1. Make all methods asynchronous — e.g. (async () => console.log(await sodium.randombytes_buf(1)))(). (This is what I chose to do in the libraries listed in New emscripten features #94.)

  2. Make a .ready promise that can be waited on and leave the otherwise synchronous API as-is (with exceptions thrown when called before .ready is resolved) — e.g. (async () => { await sodium.ready; console.log(sodium.randombytes_buf(1)); })().

  3. A hybrid approach, where during the initial compilation all methods behave as in solution 1, but .ready is also exposed and after .ready is resolved all methods switch to behaving as in solution 2 — e.g. (async () => { console.log(await sodium.randombytes_buf(1)); await sodium.ready; console.log(sodium.randombytes_buf(1)); })(). The only notable usability issue I see here is that anyone using .then()/.catch() on libsodium methods (rather than await) would need to wrap them in Promise.resolve() each time.

  4. A variant of solution 3 where the synchronous methods throw exceptions when called before .ready resolves (rather than returning promises), and to use methods asynchronously you'd have to add _async — e.g. (async () => console.log(await sodium.randombytes_buf_async(1)))().

@creationix
Copy link

@buu700 I think changing behavior after a ready function is asking for trouble. I like option 2.

@jedisct1
Copy link
Owner Author

Option 2 would be fantastic. This is the most conservative one, but being in a state where some functions work and some don't feels indeed a bit weird. Option 2 can also be seamlessly used in legacy environments that don't support promises.

@creationix
Copy link

Also I don't like the performance hit of having to run everything through the Promise system. Promises require that they always resolve or reject of a subsequent turn of the event loop. This means you can get all kinds of race conditions as well as unneeded overhead.

@buu700
Copy link
Contributor

buu700 commented Sep 29, 2017

Awesome, sounds good to me; I'll go with option 2. (That was my preference as well, but I thought 3/4 might've been a potentially interesting compromise if there'd been disagreement.)

@joepie91
Copy link

I'm not sure I see why the WASM needs to be loaded asynchronously - there are loaders like this one that can bundle .wasm files directly in a bundler, sidestepping the issue of dealing with asynchronous loading of code.

How this can deal with various usecases:

  1. Bundler usage - instruct the user to use a WASM loader for their bundler of choice (as is currently done for eg. JSON loaders), and bundle the WASM directly into the application bundle.
  2. Stand-alone usage - either provide a distribution that uses the Emscripten-provided loader, or produce a stand-alone build of the library using a bundler (that is exposed as a global to the application).
  3. Node.js usage - assuming that this needs to be covered as well (although I feel that really, native bindings should be used there), this can be done by having a separate entry point entirely. Various bundlers implement support for eg. the browser field to specify a separate path for browser vs. Node entry.

Also I don't like the performance hit of having to run everything through the Promise system. Promises require that they always resolve or reject of a subsequent turn of the event loop. This means you can get all kinds of race conditions as well as unneeded overhead.

Are you referring to option 1? Promise overhead is negligible, in particular if it only concerns the loading stage, but "unnecessary async for every function call" is a possible concern. I don't see any issues with using Promises for a loading stage, though, since that's going to be asynchronous anyway.

@buu700
Copy link
Contributor

buu700 commented Sep 29, 2017

The issue isn't building; the wasm compilation API is asynchronous. (There is a synchronous compilation API, but it's only supported for use with very small modules and it's bad for performance because it blocks the thread.)

As far as promise overhead, it sounded like he was referring to option 1, e.g. the performance of a loop of function calls could be measurably affected by each iteration taking place on a separate event loop tick.

@jedisct1
Copy link
Owner Author

Your changes have been merged, and I tried to build the unified asmjs+webasm libsodium.js files.

The resulting files are now in https://github.com/jedisct1/libsodium.js/tree/master/dist

(without optimizations/packing)

Trying to load libsodium.js as a module now fails with onAbort() being called on an undefined object.

Any idea why?

@buu700
Copy link
Contributor

buu700 commented Sep 29, 2017

Looks like the JS in emscripten.sh is getting broken by emscripten inserting var Module; a bit further down. There may be a better workaround, but this seems to fix it:

diff --git a/dist-build/emscripten.sh b/dist-build/emscripten.sh
index 34af9313..9b22d83c 100755
--- a/dist-build/emscripten.sh
+++ b/dist-build/emscripten.sh
@@ -77,11 +77,14 @@ if [ "$DIST" = yes ]; then
     if (typeof Module === 'undefined') {
       var Module = {};
     }
+    var _Module = Module;
     Module.ready = new Promise(function (resolve, reject) {
+      var Module = _Module;
       Module.onAbort = reject;
       Module.onRuntimeInitialized = function () { resolve(); };
       $(cat "${PREFIX}/lib/libsodium.wasm.tmp.js")
     }).catch(function () {
+      var Module = _Module;
       Module.onAbort = undefined;
       Module.onRuntimeInitialized = undefined;
       $(cat "${PREFIX}/lib/libsodium.asm.tmp.js" | sed 's|use asm||g')

After that, the three instances of libsodium[\"_\" in wrapper/build-wrappers.js are an issue. Just removing the one in exportFunctions works fine (although you may want to move that check into the functions themselves so that users get more helpful error messages than "libsodium is undefined" when calling things without waiting on sodium.ready), but for the ones in exportConstants it looks like you'd need to either convert all constants in the public API into functions or leave them undefined initially and then set them when resolving sodium.ready.

After removing the function check and getting rid of constants entirely, it seems to be working as expected based on a quick test of sodium.crypto_hash.

@buu700
Copy link
Contributor

buu700 commented Sep 29, 2017

Also, just committed fixes for some edge case issues that I caught while testing this to the emscripten PR, so you should pull the latest there before building this again.

@jedisct1
Copy link
Owner Author

All these symbols should indeed be defined only after sodium.ready resolves. So libsodium-wrappers should have its .ready promise as well.

@bufke
Copy link

bufke commented Oct 2, 2017

Happy to test the changes out with a webpack project. Wish I could help more.

@jedisct1
Copy link
Owner Author

jedisct1 commented Oct 4, 2017

Seems to be working awesome!

Having the initially unusable symbols in the same namespace as the .ready property with the promise feels a bit weird though.

What do you think about having the module return the promise, which resolves to an object with all the symbols?

@jedisct1
Copy link
Owner Author

jedisct1 commented Oct 4, 2017

The password hashing tests fails because TOTAL_MEMORY is too low.

What would be a clean way to set TOTAL_MEMORY to something minimal, but still allow users to override it?

@buu700
Copy link
Contributor

buu700 commented Oct 4, 2017

Nice! I see that you've just made a bunch of commits; is there anything I can do to help? (The promise thing should be pretty easily doable by changing what's currently ready to exports and moving everything else inside of it.)

As far as TOTAL_MEMORY, not sure offhand aside from the obvious ALLOW_MEMORY_GROWTH option. I think emscripten incoming introduced a higher minimum memory requirement than the current master (up to 16 MB), which may or may not be WebAssembly-related, but not 100% sure on the details of that offhand or whether it would've affected libsodium.

@jedisct1
Copy link
Owner Author

jedisct1 commented Oct 4, 2017

Module['TOTAL_MEMORY'] can be set prior to loading the libsodium module, but I don't see a clean way to do that from the wrapper.

If you could help with that, and maybe the promise thing, that would be great.

But we already have something that works pretty well now. This is fantastic, thanks a ton for your help! Hope your Emscripten PRs will finally be merged soon!

@buu700
Copy link
Contributor

buu700 commented Oct 4, 2017

Ah, do you mean you'd want a way to increase the memory limit at run-time in the password hashing methods? I've run into issues with that before (unrelated to WebAssembly) when trying to test the sensitive mem limit with the default build. Not sure if there's any better way to do that than ALLOW_MEMORY_GROWTH. (Or if you just mean you'd want a way for users to set TOTAL_MEMORY before the module is loaded, you could set Module.TOTAL_MEMORY to (window|self|global).sodium.totalMemory during the initial load, similarly to how onload is handled.)

As far as the promise thing, I'll do that and send a PR right now.

Thanks / not a problem, and thanks for crediting me in the authors section of the readme!

Also, did you mean to delete the dist folder as part of 4bb6bf4?

@jedisct1
Copy link
Owner Author

jedisct1 commented Oct 4, 2017

ALLOW_MEMORY_GROWTH apparently has performance implications, so it's probably better to avoid it. But letting uses set Module.TOTAL_MEMORY during the initial load would work for people who need to use password hashing functions.

The dist folder is coming back with the new promise thing :)

@buu700
Copy link
Contributor

buu700 commented Oct 6, 2017

For anyone else who wants to test libsodium.js in its current state (WebAssembly with asm.js fallback):

$ mkdir sodiumtest ; cd sodiumtest ; mkdir node_modules ; git clone --depth 1 https://github.com/jedisct1/libsodium.js node_modules/libsodium ; cp -a node_modules/libsodium node_modules/libsodium-wrappers ; cp -f node_modules/libsodium/package-libsodium.json node_modules/libsodium/package.json ; cp -f node_modules/libsodium/package-libsodium-wrappers.json node_modules/libsodium-wrappers/package.json ; node

> const sodium = require('libsodium-wrappers'); (async () => { await sodium.ready; console.log(sodium.crypto_hash('balls')); })();

@jedisct1
Copy link
Owner Author

jedisct1 commented Oct 6, 2017

One thing I don't like about this is the fact that sodium is not in the current namespace. So I'd prepend something like const sodium = sodium in the promise resolution function to avoid useless lookups every time the object is used.

Having the promise return the object would make this more natural.

@buu700
Copy link
Contributor

buu700 commented Oct 6, 2017

Well, in my example/test, the require could go inside the async function; but yeah, that would definitely be a bit more natural.

@jedisct1
Copy link
Owner Author

Alright, time to close this ticket :)

Thanks a ton, @buu700 , you're a hero!

@buu700
Copy link
Contributor

buu700 commented Oct 13, 2017

Awesome, not a problem at all, and thanks!

@buu700
Copy link
Contributor

buu700 commented Oct 16, 2017

@jedisct1, the SINGLE_FILE emscripten PR finally just got merged. There were a few small bugs fixed near the end, so you should switch over to the latest upstream incoming branch before the next time you rebuild libsodium.js.

@mrlubos
Copy link

mrlubos commented Feb 22, 2018

Anyone here using libsodium with Jest? The ready event never fires for me in jsdom environment.

@stevelacey
Copy link

@jedisct1 did you settle on a way to set TOTAL_MEMORY?

@jedisct1
Copy link
Owner Author

No, did you?

@stevelacey
Copy link

@jedisct1 no 😅 is there a way currently? I am getting memory errors for anything over 8mb, and I think that’s the reason?

@jedisct1
Copy link
Owner Author

Yes, I think you still need the sumo build to allocate more.

This can be solved, but I don't know how :)

@stevelacey
Copy link

stevelacey commented Oct 26, 2018 via email

@stevelacey
Copy link

Alright worked it out, as above you can up TOTAL_MEMORY via setting it on the window:

window.sodium = {totalMemory: 134217728};

@bufke
Copy link

bufke commented Oct 26, 2018

Does window.sodium work now? Last time I checked it did not. I described my workaround here. This was my preferred approach because I could use the smaller non sumo package but with enough memory for pwhash with MEMLIMIT set to 33554432.

@stevelacey
Copy link

stevelacey commented Oct 27, 2018

@bufke seems to, how did it not work? I get an abort error in the console, which there's also an outstanding issue for, but other than that my problem appears to be gone

Note: saw your workaround and attempted to follow on Ubuntu but it crashed out at some point 😕 way beyond my understanding anyway

@chicoxyzzy
Copy link

chicoxyzzy commented Mar 16, 2023

Is it still possible to load libsodium synchronously? It would be awesome to have a possibility to do it with new WebAssembly.Instance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants