Skip to content

Optionally call main() in WASI reactors as a convenience. #133

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

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

PiotrSikora
Copy link
Member

WASI reactors differ from WASI commands in that they have multiple
entrypoints (i.e. proxy_on_* callbacks) instead of only main().

Currently, each Proxy-Wasm SDK uses different approch to startup:

  • AssemblyScript SDK uses Wasm's start function.

  • C++ SDK creates WASI reactor with global C++ constructors taking
    care of early initialization and registration of plugins.

  • Rust SDK creates Wasm library, and suggests (via examples) using
    _start() function called at startup to do early initialization.
    Unfortunately, this is the same function name that WASI commands
    are using, which means that WASI constructors cannot be injected
    and executed at startup.

  • TinyGo SDK creates WASI command and calls main() at startup, but
    it doesn't exit after main() function returns.

Calling main() in WASI reactors would allow us to prepare for when
they are stablized in Rust, and to have a non-breaking fallback in
case TinyGo decides to exit after main() function returns.

Signed-off-by: Piotr Sikora [email protected]

WASI reactors differ from WASI commands in that they have multiple
entrypoints (i.e. proxy_on_* callbacks) instead of only main().

Currently, each Proxy-Wasm SDK uses different approch to startup:

- AssemblyScript SDK uses Wasm's start function.

- C++ SDK creates WASI reactor with global C++ constructors taking
  care of early initialization and registration of plugins.

- Rust SDK creates Wasm library, and suggests (via examples) using
  _start() function called at startup to do early initialization.
  Unfortunately, this is the same function name that WASI commands
  are using, which means that WASI constructors cannot be injected
  and executed at startup.

- TinyGo SDK creates WASI command and calls main() at startup, but
  it doesn't exit after main() function returns.

Calling main() in WASI reactors would allow us to prepare for when
they are stablized in Rust, and to have a non-breaking fallback in
case TinyGo decides to exit after main() function returns.

Signed-off-by: Piotr Sikora <[email protected]>
@PiotrSikora PiotrSikora requested a review from mathetake as a code owner March 1, 2021 08:27
@PiotrSikora
Copy link
Member Author

We didn't guard _start() or _initialize() previously, but perhaps this should be guarded on the ABI v0.2.2 that @Shikugawa needs for cluster name in gRPC calls?

@PiotrSikora
Copy link
Member Author

We didn't guard _start() or _initialize() previously, but perhaps this should be guarded on the ABI v0.2.2 that @Shikugawa needs for cluster name in gRPC calls?

On the other hand, we already call main() in TinyGo SDK, since it's a WASI command, so perhaps this isn't a change after all...

@mathetake
Copy link
Contributor

mathetake commented Mar 2, 2021

Looks good to me because this seems harmless to add. But as for TinyGo, this won't be a change since I wouldn't export main function literally in TinyGo's WASI target even when we start calling proc_exit at the end of _start. Instead, maybe another name would be adopted something like __tinygo_init or _initialize since calling user defined main alone is not enough because we have to call func init() {} functions in all packages used in programs. I'm the author of WASI target so hopefully I could have a bit of control over the decision.

@mathetake
Copy link
Contributor

Ah - I was confused. Maybe we would export _initialize and main in TinyGo since the separation on this line (https://github.com/tinygo-org/tinygo/blob/release/src/runtime/scheduler_none.go#L22) to have _initialize and main makes sense to me, even though this is something to do with TinyGo's core so this may not be accepted by the community.

@PiotrSikora
Copy link
Member Author

Looks good to me because this seems harmless to add. But as for TinyGo, this won't be a change since I wouldn't export main function literally in TinyGo's WASI target even when we start calling proc_exit at the end of _start. Instead, maybe another name would be adopted something like __tinygo_init or _initialize since calling user defined main alone is not enough because we have to call func init() {} functions in all packages used in programs. I'm the author of WASI target so hopefully I could have a bit of control over the decision.

Ah - I was confused. Maybe we would export _initialize and main in TinyGo since the separation on this line (https://github.com/tinygo-org/tinygo/blob/release/src/runtime/scheduler_none.go#L22) to have _initialize and main makes sense to me, even though this is something to do with TinyGo's core so this may not be accepted by the community.

TinyGo generates WASI command, which looks like this (pseudocode):

fn _start() {
  init_heap();
  global_constructors();
  main();
}

But in theory WASI command should look something like:

fn _start() {
  init_heap();
  global_constructors();
  retrun_value = main();
  global_destructors();
  proc_exit(return_value);
}

i.e. WASI command should exit after main() completes.

If TinyGo added support for WASI reactors (it should!), then it would look something like this:

fn _initialize() {
  init_heap();
  global_constructors();
}

So, my comment regarding main() and TinyGo was that if TinyGo ever added support for WASI reactors, then it would stop automatically calling main(), since WASI reactors don't do that, and then from the Proxy-Wasm plugin developer point of view, existing TinyGo plugins would stop working since main() would be no longer called.

Does it make sense?

@mathetake
Copy link
Contributor

The only thing unclear to me is that calling proc_exit looks not specified in WASI spec, even though we should do that according to the tradition. Do you have any pointer to this?

But yeah, your comment finally totally makes sense to me, and I will work on reactor support in TinyGo in order to make TinyGo more WASI spec compatible. Thanks for the clarification!

@mathetake
Copy link
Contributor

mathetake commented Mar 2, 2021

Ah - one more thing: maybe we would call main in WASI reactors' _initialize without exiting afterwards. That means

WASI commands would look like

fn _start() {
  init_heap();
  global_constructors();
  retrun_value = main();
  global_destructors();
  proc_exit(return_value);
}

WASI reactors would be like

fn _initialize() {
  init_heap();
  global_constructors();
  main(); // not exit afterwards
}

and then this PR would have no affect on TinyGo. This is kind of a design decision in TinyGo so I am not sure how this would look like when the WASI reactor support lands.

@PiotrSikora
Copy link
Member Author

The only thing unclear to me is that calling proc_exit looks not specified in WASI spec, even though we should do that according to the tradition. Do you have any pointer to this?

I don't think it's defined anywhere, WASI spec might have even more tribal knowledge than Proxy-Wasm spec :)

But if you look at Emscripten implementation, this is crt1 for WASI command:

void _start(void) {
  if (__wasm_call_ctors) {
    __wasm_call_ctors();
  }

  /*
   * Will either end up calling the user's original zero argument main directly
   * or our __original_main fallback in __original_main.c which handles
   * populating argv.
   */
  int r = __original_main();

  exit(r);
}

and this is crt1 for WASI reactor:

void _initialize(void) {
  if (__wasm_call_ctors) {
    __wasm_call_ctors();
  }
}

Ah - one more thing: maybe we would call main in WASI reactors' _initialize without exiting afterwards. That means

WASI commands would look like

fn _start() {
  init_heap();
  global_constructors();
  retrun_value = main();
  global_destructors();
  proc_exit(return_value);
}

WASI reactors would be like

fn _initialize() {
  init_heap();
  global_constructors();
  main(); // not exit afterwards
}

No, no, no! WASI reactors have multiple entrypoints, but none is called as part of initialization (that's the whole difference between commands and reactors), so we don't want to call main() from within _initialize().

What we do in this PR is that we call main() (if it's exported by the plugin) after WASI reactor is initialized.

and then this PR would have no affect on TinyGo. This is kind of a design decision in TinyGo so I am not sure how this would look like when the WASI reactor support lands.

This PR doesn't affect TinyGo at this point.

Currently, TinyGo creates WASI command (using _start function), and this PR doesn't change that code path at all.

If TinyGo adds support for WASI reactors (using _initialize function), then the result of this PR is that existing plugins will work without any changes, because we're going to call main() (if it's exported by the plugin) after WASI rector is initialized. Without this PR, they would stop working, since you're hooking into SDK in the main() function.

@mathetake
Copy link
Contributor

WASI reactors have multiple entrypoints, but none is called as part of initialization (that's the whole difference between commands and reactors), so we don't want to call main() from within _initialize().

OK, finally everything looks clear to me, and thanks for bearing with me. (And I am now thinking that I should have depended on func init(), not on func main() for plugin initialization)

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

Successfully merging this pull request may close these issues.

2 participants