Module['onRuntimeInitializeFailed'] #5357

buu700 · 2017-07-03T18:56:08Z

As discussed in #5338.

kripken · 2017-07-03T21:05:02Z

src/preamble.js

@@ -2509,7 +2509,7 @@ function integrateWasmJS(Module) {
      } else if (curr === 'interpret-asm2wasm' || curr === 'interpret-s-expr' || curr === 'interpret-binary') {
        if (exports = doWasmPolyfill(global, env, providedBuffer, curr)) break;
      } else {
-        throw 'bad method: ' + curr;
+        Module['quit'](1, 'bad method: ' + curr);


I think calling quit here is not the best thing. quit is the lowest layer here, and by calling it, you are forced to add logic into shell.js which is higher than it should be. Instead, how about calling abort here, and in abort (in postamble.js), checking if onRuntimeInitializeFailed exists and if so, call it?

Got it, thanks; just took care of this in a force push over the original commit.

kripken · 2017-07-03T21:06:29Z

This will also need testing. Perhaps add other.test_runtime_initialize_failed, in which we cause init to fail somehow (e.g., maybe delete the wasm file before running the code?)

kripken · 2017-07-03T23:12:34Z

src/postamble.js

@@ -316,6 +316,10 @@ Module['exit'] = Module.exit = exit;
 var abortDecorators = [];

 function abort(what) {
+  if (Module['onRuntimeInitializeFailed']) {
+    setTimeout(function () { Module['onRuntimeInitializeFailed'](what); }, 0);


why is there a setTimeout here? it needs an explanation, but also I don't think it's needed, unless I'm missing something?

It's probably not really needed; I can either remove it or add a comment depending on your preference. The reason I put it in a setTimeout was to avoid having an exception in the callback interfere with the rest of abort or anything else that may come afterwards (just being conservative and erring on the side of not changing existing behavior).

Ok, let's take it out then, I think it adds unnecessary complexity.

Cool, done.

kripken · 2017-07-03T23:15:04Z

src/postamble.js

@@ -316,6 +316,10 @@ Module['exit'] = Module.exit = exit;
 var abortDecorators = [];

 function abort(what) {
+  if (Module['onRuntimeInitializeFailed']) {


if we want this callback to be just for runtime initialization failures, then we should erase Module['onRuntimeInitializeFailed'] after initialization succeeds (so aborts later on don't cause it to be called).

alternatively, if you want a general callback when the problem aborts for any reason, including after initialization, then we should rename this. (or, perhaps we should just recommend people set Module.abort to this callback? right now we'd override that, but we could change that)

Cool, good catch; just took care of this.

I wonder though if we shouldn't add onAbort instead of this. Would it handle your use case? If so, then since it handles more use cases too, and avoids the complexity of needing to unset onRuntimeInitializeFailed, it seems superior.

Hm, yeah, I wouldn't have any problem with that. As long as keeping my change of calling abort in the WebAssembly.instantiate catch isn't an issue, it'd be practically the same for my purposes.

Yeah, I think that change is more correct regardless - we should call abort when forced to abort execution. (It's more important after execution starts, since abort makes sure we stop running events afterwards; but it's better for consistency to do it in all places.)

Perfect, makes sense to me. Just pushed a commit that changes this to onAbort.

…mscripten-core#5357)

…on (emscripten-core#5357)

kripken · 2017-07-05T21:47:16Z

tests/test_other.py

+
+    expectedOutput = "Module['onAbort'] was called"
+
+    os.remove('a.out.wasm')


please add a comment on this line, about it making startup fail

kripken · 2017-07-05T21:47:39Z

tests/test_other.py

+    f.write("Module['onAbort'] = function () { console.log('%s') }" % expectedOutput)
+    f.close()
+
+    devnull = open(os.devnull, 'w')


can we remove the devnull parts? it's out if stderr is logged to the console as the test runs (easier to debug, when necessary)

kripken · 2017-07-05T21:48:24Z

Nice, lgtm with those test changes. Also, we need you in AUTHORS to merge (which I think is in your other PR, but not merged yet, so perhaps add it here if you want this merged earlier).

buu700 · 2017-07-05T22:05:52Z

Nice, thanks! Just pushed those test changes and the AUTHORS update.

kripken · 2017-07-05T23:38:59Z

Does the test pass for you? I get

Module['onAbort'] = function () { console.log('Module['onAbort'] was called') }
                                              ^^^^^^^^^

SyntaxError: missing ) after argument list

I think you need to change one set of quotation marks there to double-quotes.

buu700 · 2017-07-05T23:41:54Z

Oops, sorry about that. I'll fix it and confirm that it passes before pushing again.

kripken · 2017-07-05T23:45:42Z

Another concern I just realized is that your test assumes the default js engine used by run_js has wasm support. That's typically node.js, and it doesn't have wasm support unless it's very recent. Running with BINARYEN_METHOD="interpret-binary" would be better, as the interpreter is guaranteed to work.

buu700 · 2017-07-05T23:49:18Z

Got it, thanks; I'll fix that too.

buu700 · 2017-07-06T00:05:48Z

How should callbacks like onAbort and onRuntimeInitialized be defined when the loading method is synchronous? The test worked with native-wasm because it got to Module['onAbort'] = ... before abort was actually called, but that isn't the case with interpret-binary.

kripken · 2017-07-06T00:29:45Z

Synchronous loading of wasm should abort if the wasm binary is not present (i.e. I don't think it being synchronous or asynchronous should make a difference in error handling). If that's not currently an abort, then it might need to be turned into one.

Should be easy, I think. If not, maybe there's another way to test this. Another option which is more work would be to add infrastructure in the test harness to check which js engine(s) have wasm support, then use that engine (or skip) in this test. We need that anyhow, and I could look into doing it soon if it would help here.

buu700 · 2017-07-06T00:36:41Z

It is aborting if synchronous loading/compilation fails, but the issue is that I don't know how my onAbort callback is meant to be correctly attached to Module in that case.

Right now, I'm just appending Module['onAbort'] = ... to the end of the file, which never gets executed because the call to abort happens before that line is ever reached.

kripken · 2017-07-06T00:43:33Z

Oh, ok. Then you should define the Module before the program, instead of at the end, prepend something like

var Module = {
  onAbort: ..
};

buu700 · 2017-07-06T00:45:33Z

Ah, thanks. I'd actually tried that and it didn't seem to work, but maybe something else was causing the test to fail; I'll unstash it and try again.

buu700 · 2017-07-06T01:19:22Z

Okay, so the issue I had before was that it was asserting a return code of 0 by default. After disabling that, now the issue is that it's returning no output for some reason (Exception: Expected to find 'Module.onAbort was called' in ''). Any ideas?

This is what the test currently looks like:

  def test_on_abort(self):
    cmd = [PYTHON, EMCC, path_from_root('tests', 'hello_world.c'), '-s', 'WASM=1', '-s', 'BINARYEN_METHOD="interpret-binary"']

    self.clear()
    proc = Popen(cmd, stdout=PIPE, stderr=PIPE)
    output, err = proc.communicate()

    expectedOutput = 'Module.onAbort was called'

    # trigger onAbort by intentionally causing startup to fail
    os.remove('a.out.wasm')

    f = open('a.out.js', 'r')
    js = f.read()
    f.close()
    f = open('a.out.js', 'w')
    f.write("var Module = {onAbort: function() { console.log('%s') }};\n" % expectedOutput)
    f.write(js)
    f.close()

    self.assertContained(expectedOutput, run_js('a.out.js', assert_returncode=None))

kripken · 2017-07-06T02:46:50Z

Is anything printed to the console through stderr? Maybe it's throwing an error before it reaches your code. You should see it printed in your console, or could tell run_js to redirect stderr to stdout (something like stderr=subprocess.stdout).

buu700 · 2017-07-06T03:00:51Z

Thanks! That was it. Apparently I'd misremembered the "bad method" abortion as a synchronous compilation abortion. Added an try/catch to abort in getBinary and now the test is passing.

kripken · 2017-07-06T05:02:30Z

Looks good, thanks!

dschuff · 2017-07-06T17:50:23Z

This is nice, can we update the emscripten docs as well?

kripken · 2017-07-06T17:53:30Z

I'll do that, I have some other followups here as well.

kripken reviewed Jul 3, 2017

View reviewed changes

Module['onRuntimeInitializeFailed'] (emscripten-core#5357)

8030e3b

buu700 force-pushed the onruntimeinitializefailed branch from 4cf6fe1 to 8030e3b Compare July 3, 2017 23:03

kripken reviewed Jul 3, 2017

View reviewed changes

test_runtime_initialize_failed (emscripten-core#5357)

e845dac

buu700 added a commit to buu700/emscripten that referenced this pull request Jul 3, 2017

erase Module['onRuntimeInitializeFailed'] after runtime init success (e…

45f488e

…mscripten-core#5357)

buu700 force-pushed the onruntimeinitializefailed branch from 45f488e to c3f533d Compare July 3, 2017 23:45

buu700 added a commit to buu700/emscripten that referenced this pull request Jul 3, 2017

erase Module['onRuntimeInitializeFailed'] after runtime init completi…

c3f533d

…on (emscripten-core#5357)

erase Module['onRuntimeInitializeFailed'] after runtime init completi…

1f73bb5

…on (emscripten-core#5357)

buu700 force-pushed the onruntimeinitializefailed branch from c3f533d to 1f73bb5 Compare July 3, 2017 23:54

minor cleanup (emscripten-core#5357)

80826bc

buu700 added a commit to buu700/emscripten that referenced this pull request Jul 5, 2017

onRuntimeInitializeFailed -> onAbort (emscripten-core#5357)

9c11d41

onRuntimeInitializeFailed -> onAbort (emscripten-core#5357)

47875d6

buu700 force-pushed the onruntimeinitializefailed branch from 9c11d41 to 47875d6 Compare July 5, 2017 21:44

kripken reviewed Jul 5, 2017

View reviewed changes

buu700 added a commit to buu700/emscripten that referenced this pull request Jul 5, 2017

misc test cleanup (emscripten-core#5357)

a89eca1

buu700 added a commit to buu700/emscripten that referenced this pull request Jul 5, 2017

add myself to AUTHORS (emscripten-core#5296 + emscripten-core#5357)

e42bc90

buu700 added 2 commits July 5, 2017 18:09

misc test cleanup (emscripten-core#5357)

70aa0be

add myself to AUTHORS (emscripten-core#5296 + emscripten-core#5357)

c36e4d7

buu700 force-pushed the onruntimeinitializefailed branch from e42bc90 to c36e4d7 Compare July 5, 2017 22:09

buu700 added 2 commits July 5, 2017 22:58

abort on getBinary failure (emscripten-core#5357)

960ea42

test fix (emscripten-core#5357)

ced910e

kripken merged commit 8404d94 into emscripten-core:incoming Jul 6, 2017

buu700 deleted the onruntimeinitializefailed branch July 6, 2017 05:16

buu700 mentioned this pull request Jul 6, 2017

New emscripten features jedisct1/libsodium.js#94

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module['onRuntimeInitializeFailed'] #5357

Module['onRuntimeInitializeFailed'] #5357

buu700 commented Jul 3, 2017

kripken Jul 3, 2017

buu700 Jul 3, 2017

kripken commented Jul 3, 2017

kripken Jul 3, 2017

buu700 Jul 3, 2017 •

edited

Loading

kripken Jul 5, 2017

buu700 Jul 5, 2017

kripken Jul 3, 2017

buu700 Jul 3, 2017

kripken Jul 5, 2017

buu700 Jul 5, 2017

kripken Jul 5, 2017

buu700 Jul 5, 2017

kripken Jul 5, 2017

kripken Jul 5, 2017

kripken commented Jul 5, 2017

buu700 commented Jul 5, 2017 •

edited

Loading

kripken commented Jul 5, 2017

buu700 commented Jul 5, 2017

kripken commented Jul 5, 2017

buu700 commented Jul 5, 2017

buu700 commented Jul 6, 2017

kripken commented Jul 6, 2017

buu700 commented Jul 6, 2017

kripken commented Jul 6, 2017

buu700 commented Jul 6, 2017

buu700 commented Jul 6, 2017 •

edited

Loading

kripken commented Jul 6, 2017

buu700 commented Jul 6, 2017

kripken commented Jul 6, 2017

dschuff commented Jul 6, 2017

kripken commented Jul 6, 2017


		expectedOutput = "Module['onAbort'] was called"

		os.remove('a.out.wasm')

Module['onRuntimeInitializeFailed'] #5357

Module['onRuntimeInitializeFailed'] #5357

Conversation

buu700 commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Jul 3, 2017

Choose a reason for hiding this comment

buu700 Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Jul 5, 2017

buu700 commented Jul 5, 2017 • edited Loading

kripken commented Jul 5, 2017

buu700 commented Jul 5, 2017

kripken commented Jul 5, 2017

buu700 commented Jul 5, 2017

buu700 commented Jul 6, 2017

kripken commented Jul 6, 2017

buu700 commented Jul 6, 2017

kripken commented Jul 6, 2017

buu700 commented Jul 6, 2017

buu700 commented Jul 6, 2017 • edited Loading

kripken commented Jul 6, 2017

buu700 commented Jul 6, 2017

kripken commented Jul 6, 2017

dschuff commented Jul 6, 2017

kripken commented Jul 6, 2017

buu700 Jul 3, 2017 •

edited

Loading

buu700 commented Jul 5, 2017 •

edited

Loading

buu700 commented Jul 6, 2017 •

edited

Loading