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

wasi: introduce initial WASI support #30258

Closed
wants to merge 2 commits into from
Closed

Conversation

@cjihrig
Copy link
Contributor

cjihrig commented Nov 5, 2019

This PR introduces experimental support for the WebAssembly System Interface (WASI). This PR adds a new core module, wasi, and a new C dependency, uvwasi, that implements the WASI system call API on top of libuv. uvwasi currently implements version wasi_unstable_preview0 of the WASI API. It's worth pointing out that WASI itself is still evolving, and uvwasi is still young. There will be bugs and breaking changes.

Up to this point, development has been happening in the uvwasi repository, and the nodejs/wasi fork. The commit metadata currently lists the individuals that have written and reviewed code in those two repositories.

I'm opening this PR because I can't realistically continue to dedicate so much time to this work (which started in July) indefinitely without confirmed buy in from the project. Because this introduces a new core module, cc: @nodejs/tsc. Also cc: @nodejs/wasi and @nodejs/libuv.

Refs: #27850

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 5, 2019

Can you call the exposed module wasi_unstable_preview0 or wasi/unstable_preview0 instead of wasi?

@guybedford

This comment has been minimized.

Copy link
Contributor

guybedford commented Nov 5, 2019

Given that this whole feature is behind a flag I think it makes sense to adopt a name that can be considered as the final name here.

@devsnek would you consider giving up your wasi package on npm and renaming your package to something else?

Otherwise, if it helps, I have wasi_unstable on npm and would be glad to give that up for this.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 5, 2019

i think the name should match the phase we're implementing. if we want to use wasi or wasi/unstable_preview0 i will cede my package.

Copy link
Contributor

guybedford left a comment

Amazing work @cjihrig, thanks so much for your efforts and it would be great to see this in Node.js continuing to be iterated upon further.

Since there is no wasi: subsystem, how about we use a generic wasm: subsystem for this PR to run against CI for now. I've posted nodejs/core-validate-commit#82.

@devsnek the reason for using a versioned import in WASI output is to ensure backwards compatibility can be maintained. That is allowing different WASI binaries to coexist in the wild.

Using the same versioning scheme for Node.js core conflates the WASI API version with the WASI Node.js API version, which are two very separate things. Node.js users tie their WASI version to the version of the Node.js overall - we don't version core modules for this reason. So there is no benefit to versioning any WASI builtin module in core, although an unstable marker could be considered certainly.

doc/api/wasi.md Show resolved Hide resolved
lib/wasi.js Show resolved Hide resolved
doc/api/wasi.md Outdated Show resolved Hide resolved
@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 5, 2019

@guybedford i'm aware why the versions are the way they are, i just want to ensure maximum verbosity and breakage while this isn't stable.

Copy link
Member

mcollina left a comment

rubberstamp LGTM

Would this require some changes to our CI to be able to run the tests?

@saghul
saghul approved these changes Nov 5, 2019
Copy link
Member

saghul left a comment

Excellent work!

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Nov 5, 2019

@devsnek, would changing the CLI flag to --experimental-wasi-unstable-preview0 and using wasi as the module name work for you? We should avoid changing the top level module name because of the impact on the npm ecosystem.

Would this require some changes to our CI to be able to run the tests?

@mcollina no, as long as we check in the .wasm files. Recompiling the WASI tests from source would require the CI or user to have wasi-libc set up though.

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Nov 5, 2019

no, as long as we check in the .wasm files. Recompiling the WASI tests from source would require the CI or user to have wasi-libc set up though.

That would work for now, however I think we should aim for it in the future (maybe with just one env?), maybe after this land you might want to open a tracking issue on build.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 5, 2019

@cjihrig that works too I guess

Copy link

galvesribeiro left a comment

Awesome! LGTM

Copy link
Member

addaleax left a comment

I think this needs to come with a license file checked into the repo and added to tools/license-builder.sh (and then have that script run)?

uvwasi_fd_t id;
uv_file fd;
char path[PATH_MAX_BYTES];
char real_path[PATH_MAX_BYTES];

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 5, 2019

Member

Is uvwasi ABI-stable at this point, or are breaking changes fine? I’d maybe open a PR against it to make these pointers, both to avoid artificial limitations on the max path lengths and to avoid the large memory overhead that this is likely to have?

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 5, 2019

Author Contributor

Breaking changes are totally fine at this point 😄

src/node_config.cc Outdated Show resolved Hide resolved
src/node_wasi.cc Outdated Show resolved Hide resolved
src/node_wasi.h Outdated Show resolved Hide resolved
src/node_wasi.cc Outdated Show resolved Hide resolved
src/node_wasi.cc Outdated Show resolved Hide resolved
src/node_wasi.cc Outdated Show resolved Hide resolved
lib/wasi.js Show resolved Hide resolved
src/node_wasi.h Outdated Show resolved Hide resolved
lib/wasi.js Outdated Show resolved Hide resolved
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 5, 2019

Since there is no wasi: subsystem, how about we use a generic wasm: subsystem for this PR to run against CI for now. I've posted nodejs/core-validate-commit#82.

Fwiw, I’d prefer to just keep using wasi:. It’s somewhat annoying that core-validate-commit makes Travis CI fail, but ignoring it should be fine.

@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Nov 5, 2019

If wasi was to become modular (which is one of the goals if I remember correctly), could selecting a subset of modules (or a specific version of wasi) be achieved through the options object passed to new WASI?

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 5, 2019

there wouldn't be a need to select a subset of functionality or version, because the namespaces are distinct to the wasm module. we can just provide all of it and the module takes what it wants.

@richardlau richardlau mentioned this pull request Nov 5, 2019
@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Nov 6, 2019

Since there is no wasi: subsystem, how about we use a generic wasm: subsystem for this PR to run against CI for now. I've posted nodejs/core-validate-commit#82.

Fwiw, I’d prefer to just keep using wasi:. It’s somewhat annoying that core-validate-commit makes Travis CI fail, but ignoring it should be fine.

wasi and wasm have been added to core-validate-commit. I've rerun the failed Travis job and it's now green.


runWASI({ test: 'cant_dotdot' });
runWASI({ test: 'clock_getres' });
runWASI({ test: 'exitcode', exitCode: 120 });

This comment has been minimized.

Copy link
@devsnek

devsnek Nov 6, 2019

Member

any reason you removed the stdin/stdout/exitcode from the file itself?

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 6, 2019

Author Contributor

Not really, I just thought it was simpler to have the values as part of the JS tests vs. parsing values out of the C file.

doc/api/wasi.md Outdated Show resolved Hide resolved
Copy link
Member

addaleax left a comment

I think all comments that I’d have considered blocking have been addressed 👍

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Nov 6, 2019

wasi and wasm have been added to core-validate-commit. I've rerun the failed Travis job and it's now green.

Thank you for taking care of that!

I think all comments that I’d have considered blocking have been addressed

I'm going to spend more time on it today - I ran out of time last night before I could get everything else addressed.

src/node_wasi.cc Outdated Show resolved Hide resolved
@cjihrig cjihrig force-pushed the cjihrig:wasi-pr branch from 372ffdc to 5165a82 Nov 11, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Nov 21, 2019

@guybedford

This comment has been minimized.

Copy link
Contributor

guybedford commented Nov 29, 2019

Is there anything holding this up from landing at this point? Or can we merge?

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 29, 2019

@cjihrig I wanted to land this but the tests don’t pass anymore, can you take a look? e.g.

=== release test-wasi-binding ===                   
Path: wasi/test-wasi-binding
wasi.js:25
      args = ArrayPrototype.map(args, (arg) => { return String(arg); });
                            ^

TypeError: [object Array] is not a function
    at Array.map (<anonymous>)
    at new WASI (wasi.js:25:29)
    at Object.<anonymous> (/home/xxxx/src/node/master/test/wasi/test-wasi-binding.js:10:14)
    at Module._compile (internal/modules/cjs/loader.js:1185:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1224:10)
    at Module.load (internal/modules/cjs/loader.js:1040:32)
    at Function.Module._load (internal/modules/cjs/loader.js:944:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47
Command: out/Release/node --experimental-wasi-unstable-preview0 /home/xxxx/src/node/master/test/wasi/test-wasi-binding.js
cjihrig and others added 2 commits Sep 4, 2019
Co-authored-by: Gus Caplan <me@gus.host>
Co-authored-by: Daniel Bevenius <daniel.bevenius@gmail.com>
Co-authored-by: Jiawen Geng <technicalcute@gmail.com>
Co-authored-by: Tobias Nießen <tniessen@tnie.de>
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
@cjihrig cjihrig force-pushed the cjihrig:wasi-pr branch from 5ec5e99 to 786b152 Nov 29, 2019
@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Nov 29, 2019

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Nov 29, 2019

I pushed a fixup commit - just changes to primordials - and kicked off a CI run.

addaleax added a commit that referenced this pull request Nov 30, 2019
Co-authored-by: Gus Caplan <me@gus.host>
Co-authored-by: Daniel Bevenius <daniel.bevenius@gmail.com>
Co-authored-by: Jiawen Geng <technicalcute@gmail.com>
Co-authored-by: Tobias Nießen <tniessen@tnie.de>
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

PR-URL: #30258
Refs: #27850
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 30, 2019

Landed in 09b1228 🎉

@addaleax addaleax closed this Nov 30, 2019
@cjihrig cjihrig deleted the cjihrig:wasi-pr branch Nov 30, 2019
targos added a commit that referenced this pull request Dec 1, 2019
Co-authored-by: Gus Caplan <me@gus.host>
Co-authored-by: Daniel Bevenius <daniel.bevenius@gmail.com>
Co-authored-by: Jiawen Geng <technicalcute@gmail.com>
Co-authored-by: Tobias Nießen <tniessen@tnie.de>
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

PR-URL: #30258
Refs: #27850
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
BridgeAR added a commit that referenced this pull request Dec 3, 2019
Notable changes:

* http:
  * Make maximum header size configurable per-stream or per-server
    (Anna Henningsen) #30570
* http2:
  * Make maximum tolerated rejected streams configurable (Denys
    Otrishko) #30534
  * Allow to configure maximum tolerated invalid frames (Denys
    Otrishko) #30534
* wasi:
  * Introduce initial WASI support (cjihrig)
    #30258

PR-URL: #30774
BridgeAR added a commit that referenced this pull request Dec 3, 2019
Notable changes:

* fs:
  * Reworked experimental recursive `rmdir()`  (cjihrig)
    #30644
    * The `maxBusyTries` option is renamed to `maxRetries`, and its
      default is set to 0. The `emfileWait` option has been removed,
      and `EMFILE` errors use the same retry logic as other errors.
      The `retryDelay` option is now supported. `ENFILE` errors are
      now retried.
* http:
  * Make maximum header size configurable per-stream or per-server
    (Anna Henningsen) #30570
* http2:
  * Make maximum tolerated rejected streams configurable (Denys
    Otrishko) #30534
  * Allow to configure maximum tolerated invalid frames (Denys
    Otrishko) #30534
* wasi:
  * Introduce initial WASI support (cjihrig)
    #30258

PR-URL: #30774
BridgeAR added a commit that referenced this pull request Dec 3, 2019
Notable changes:

* fs:
  * Reworked experimental recursive `rmdir()`  (cjihrig)
    #30644
    * The `maxBusyTries` option is renamed to `maxRetries`, and its
      default is set to 0. The `emfileWait` option has been removed,
      and `EMFILE` errors use the same retry logic as other errors.
      The `retryDelay` option is now supported. `ENFILE` errors are
      now retried.
* http:
  * Make maximum header size configurable per-stream or per-server
    (Anna Henningsen) #30570
* http2:
  * Make maximum tolerated rejected streams configurable (Denys
    Otrishko) #30534
  * Allow to configure maximum tolerated invalid frames (Denys
    Otrishko) #30534
* wasi:
  * Introduce initial WASI support (cjihrig)
    #30258

PR-URL: #30774
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 5, 2019
Co-authored-by: Gus Caplan <me@gus.host>
Co-authored-by: Daniel Bevenius <daniel.bevenius@gmail.com>
Co-authored-by: Jiawen Geng <technicalcute@gmail.com>
Co-authored-by: Tobias Nießen <tniessen@tnie.de>
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

PR-URL: nodejs#30258
Refs: nodejs#27850
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 2019
Co-authored-by: Gus Caplan <me@gus.host>
Co-authored-by: Daniel Bevenius <daniel.bevenius@gmail.com>
Co-authored-by: Jiawen Geng <technicalcute@gmail.com>
Co-authored-by: Tobias Nießen <tniessen@tnie.de>
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

PR-URL: nodejs#30258
Refs: nodejs#27850
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 2019
Notable changes:

* fs:
  * Reworked experimental recursive `rmdir()`  (cjihrig)
    nodejs#30644
    * The `maxBusyTries` option is renamed to `maxRetries`, and its
      default is set to 0. The `emfileWait` option has been removed,
      and `EMFILE` errors use the same retry logic as other errors.
      The `retryDelay` option is now supported. `ENFILE` errors are
      now retried.
* http:
  * Make maximum header size configurable per-stream or per-server
    (Anna Henningsen) nodejs#30570
* http2:
  * Make maximum tolerated rejected streams configurable (Denys
    Otrishko) nodejs#30534
  * Allow to configure maximum tolerated invalid frames (Denys
    Otrishko) nodejs#30534
* wasi:
  * Introduce initial WASI support (cjihrig)
    nodejs#30258

PR-URL: nodejs#30774
int main() {
FILE* file = fopen("/sandbox/subdir/loop1", "r");
assert(file == NULL);
assert(errno == ELOOP);

This comment has been minimized.

Copy link
@sam-github

sam-github Jan 8, 2020

Member

@cjihrig would it be possible to create these recursive symlinks dynamically during the test?

The error that is occurring above, ELOOP, is also being found by tooling that recurses the files from a nodejs/node checkout. I'm working around it, but it broke part of my work flow:

$ git ls-files -z $(git rev-parse --show-cdup) | tr '\0' '\n' | entr -crx ninja -C out/Release
entr: cannot stat 'test/fixtures/wasi/subdir/loop1': Too many levels of symbolic link

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 8, 2020

Author Contributor

@sam-github I'll send a PR later this evening.

targos added a commit that referenced this pull request Jan 14, 2020
Co-authored-by: Gus Caplan <me@gus.host>
Co-authored-by: Daniel Bevenius <daniel.bevenius@gmail.com>
Co-authored-by: Jiawen Geng <technicalcute@gmail.com>
Co-authored-by: Tobias Nießen <tniessen@tnie.de>
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

PR-URL: #30258
Refs: #27850
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.