Skip to content

Allow synchronous ccall from within asynchronous ccalls - #9400

Closed
jgautier wants to merge 5 commits into
emscripten-core:mainfrom
jgautier:jg-asyncify-sync-ccall
Closed

Allow synchronous ccall from within asynchronous ccalls#9400
jgautier wants to merge 5 commits into
emscripten-core:mainfrom
jgautier:jg-asyncify-sync-ccall

Conversation

@jgautier

@jgautier jgautier commented Sep 6, 2019

Copy link
Copy Markdown

When doing a synchronous ccall from within an asynchronous ccall with assertions on I think it is (incorrectly) throwing an error. This change makes it so it will only assert on an async ccall if there is Asyncify.currData and Asyncify.currData changed when calling the function

Example Program

// example.c
#include <emscripten.h>
#include <stdio.h>

EM_JS(void, do_async_thing, (), {
  const notAsync = cwrap('do_not_async_thing', null, []);
  Asyncify.handleSleep(function(wakeUp) {
    out("waiting for timeout");
    setTimeout(function () {
      notAsync();
      out("got the timeeout");
      wakeUp();
    }, 500)
  });
});

void do_thing () {
  puts("before");
  do_async_thing();
  puts("after");
}

void do_not_async_thing () {
  puts("not async");
}

EM_JS(void, do_thing_js, (), {
    const doAsyncThing = cwrap('do_thing', null, [], { async: true });
    doAsyncThing().then(() => {
    });
})

int main() {
  do_thing_js();
}

Compile command

emcc example2.c -s ASSERTIONS=1 -g4 -O3 -o a.html -s ASYNCIFY -s 'ASYNCIFY_IMPORTS=["do_async_thing"]' -s 'EXPORTED_FUNCTIONS=["_do_thing", "_do_not_async_thing", "_main"]'

Previously this program would fail with

before
waiting for timeout
not async
Assertion failed: The call to do_not_async_thing is running asynchronously. If this was intended, add the async option to the ccall/cwrap call.
Assertion failed: The call to do_not_async_thing is running asynchronously. If this was intended, add the async option to the ccall/cwrap call.

/Users/juliangautier/Documents/asyncify_demo/a.js:130
      throw ex;
      ^
abort(Assertion failed: The call to do_not_async_thing is running asynchronously. If this was intended, add the async option to the ccall/cwrap call.) at Error
    at jsStackTrace (/Users/juliangautier/Documents/asyncify_demo/a.js:1799:17)
    at stackTrace (/Users/juliangautier/Documents/asyncify_demo/a.js:1824:16)
    at abort (/Users/juliangautier/Documents/asyncify_demo/a.js:3853:44)
    at assert (/Users/juliangautier/Documents/asyncify_demo/a.js:627:5)
    at ccall (/Users/juliangautier/Documents/asyncify_demo/a.js:685:5)
    at /Users/juliangautier/Documents/asyncify_demo/a.js:706:12
    at Timeout._onTimeout (/Users/juliangautier/Documents/asyncify_demo/a.js:1750:177)
    at ontimeout (timers.js:475:11)
    at tryOnTimeout (timers.js:310:5)
    at Timer.listOnTimeout (timers.js:270:5)

and now passes

before
waiting for timeout
not async
after

@jgautier jgautier changed the title Jg asyncify sync ccall Allow synchronous ccall from within asynchronous ccalls Sep 6, 2019
@kripken

kripken commented Sep 6, 2019

Copy link
Copy Markdown
Member

Interesting, thanks @jgautier!

This worries me, though - currData is not actually a unique ID of an async "transaction". It's the location of the data. It may be reused over time, and in fact it would be invalid to start a second async operation with the same data while the first is still running, but I think that assertion would no longer catch things?

It's possible to design a more complex API that would handle all the above cases, but I suspect the complexity would make it not worth it?

@jgautier

Copy link
Copy Markdown
Author

@kripken I made a slight change to this that I believe covers the async and sync case properly. There is a new variable Async.currCount that gets incremented whenever allocateData is called and then this count is checked instead of checking Asyncify.currData. I tested this with the following code for doing concurrent async and the assertion fails properly.

// example.c
#include <emscripten.h>
#include <stdio.h>

EM_JS(void, do_async_thing, (), {
  const anotherAsyncThing = cwrap('do_async_thing2', null, [], { async: true });
  Asyncify.handleSleep(function(wakeUp) {
    out("waiting for timeout");
    setTimeout(function () {
      anotherAsyncThing();
      out("got the timeeout");
      wakeUp();
    }, 500)
  });
});

void do_thing () {
  puts("before");
  do_async_thing();
  puts("after");
}

EM_JS(void, do_another_async_thing, (), {
  Asyncify.handleSleep(function(wakeUp) {
    out("waiting for timeout");
    setTimeout(function () {
      out("got the timeeout");
      wakeUp();
    }, 500)
  });
});

void do_async_thing2() {
  do_another_async_thing();
}

EM_JS(void, do_thing_js, (), {
    const doAsyncThing = cwrap('do_thing', null, [], { async: true });
    doAsyncThing().then(() => {
    });
})

int main() {
  do_thing_js();
}

Let me know what you think. If this isn't the right fix and fix is too complicated do you think we should add some documentation around not allowing sync ccall while an async ccall is in progress?

@kripken

kripken commented Sep 11, 2019

Copy link
Copy Markdown
Member

If this isn't the right fix and fix is too complicated do you think we should add some documentation around not allowing sync ccall while an async ccall is in progress?

I think that might be best, yeah... this is dangerously complicated stuff already, I'm afraid to add any more.

How about documenting this limitation, and also adding some text in the assertion error that happens?

@AjayP13

AjayP13 commented Sep 11, 2019

Copy link
Copy Markdown
Contributor

@jgautier I may or may not have fixed this over in #9423 related to a different issue. It now allows sync ccalls while an async ccall is in-flight. Could you test it out on your failing case? It still may not fix this though since doing ccall/cwrap within Asyncify.handleSleep is a special case. Worth trying before relegating it to documentation on not using ccall/cwrap within Asyncify.handleSleep

@sbc100 sbc100 closed this Jan 30, 2020
@sbc100

sbc100 commented Jan 30, 2020

Copy link
Copy Markdown
Collaborator

The deletion of the incoming branch triggered the automatic closing of this branch. My apologies. This was an unexpected side effect.

Re-opening and re-targeting to master.

@sbc100 sbc100 reopened this Jan 30, 2020
@sbc100
sbc100 changed the base branch from incoming to master January 30, 2020 20:17
@stale

stale Bot commented Jan 31, 2021

Copy link
Copy Markdown

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale Bot added the wontfix label Jan 31, 2021
Base automatically changed from master to main March 8, 2021 23:49
@stale stale Bot removed the wontfix label Mar 8, 2021
@stale

stale Bot commented Apr 16, 2022

Copy link
Copy Markdown

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale Bot added the wontfix label Apr 16, 2022
@stale stale Bot closed this Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants