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

Request: mark WASI as stable #46254

Closed
cjihrig opened this issue Jan 18, 2023 · 36 comments
Closed

Request: mark WASI as stable #46254

cjihrig opened this issue Jan 18, 2023 · 36 comments
Labels
feature request Issues that request new features to be added to Node.js. wasi Issues and PRs related to the WebAssembly System Interface.

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Jan 18, 2023

What is the problem this feature will solve?

WASI support has been in Node for a few years now, but still requires a CLI flag to use. Node's WASM/WASI story appears to be pretty competitive, but people call out the fact that it requires a flag to use (https://00f.net/2023/01/04/webassembly-benchmark-2023/). From what I understand, some other projects such as Kotlin are currently using Node's WASI implementation, and Grain is planning to as well.

What is the feature you are proposing to solve the problem?

A few people (@mcollina, @guybedford) have suggested that we should unflag it at this point. I support this, but there are a few things that we would need to address:

  • We need a way to support multiple versions of WASI. snapshot1 is what we currently support, but work is underway to release snapshot2. This would require changes in Node's API, as well as the underlying WASI implementation (currently in uvwasi). On the Node side, I would recommend passing the WASI version to the WASI constructor, instead of specifying it from the command line (new WASI({ version: 'preview1' })). The uvwasi changes would be more in depth, which brings me to the next point.
  • No one is actively working on uvwasi anymore. When I originally wrote uvwasi, I was able to spend essentially as much time as I needed during working hours. That is no longer the case, and hasn't been in years now. I transferred the project into the nodejs GitHub org, hoping that other people would pick up maintenance, but that hasn't happened outside of some contributions from people from other projects. We should not stabilize WASI in Node if no one will spend any time working on it. In addition to adding support for the upcoming snapshot2, there is a new API that was (very surprisingly) added to the existing snapshot1 that would allow servers written in WASM to be supported.

What alternatives have you considered?

Leaving things as they are.

@cjihrig cjihrig added feature request Issues that request new features to be added to Node.js. wasi Issues and PRs related to the WebAssembly System Interface. labels Jan 18, 2023
@GeoffreyBooth
Copy link
Member

For me the question is, what potential breaking changes might we want to make to this API after making it stable? Only the support of the next WASI version? So if we make the desired WASI version a required part of the constructor, would that cover us?

As for the maintenance aspect, that’s a general question we should probably address separately: is it better to have an API in core that’s essentially unmaintained, versus not having it there at all. I don’t know, but I don’t think it’s specific to this. We have plenty of unmaintained APIs in core already, both experimental and stable.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 18, 2023

For me the question is, what potential breaking changes might we want to make to this API after making it stable? Only the support of the next WASI version? So if we make the desired WASI version a required part of the constructor, would that cover us?

I don't think we've shipped any breaking changes thus far - everything has been semver minor or bug fixes IIRC. I think the biggest unknown there is how WASI itself evolves. For example, they recently added a new API to an existing snapshot version without any type of version change. They could theoretically remove an API too.

@GeoffreyBooth
Copy link
Member

I think the biggest unknown there is how WASI itself evolves.

I have the same issue with JSON modules / import assertions. We unflagged that feature a long time ago (years?) and I want to mark it stable, but the TC39 proposal is sitting at Stage 3. Since the proposal/syntax itself could theoretically still change, I feel like I can’t mark our implementation of it as stable until then.

On the other hand, it’s not that once something becomes stable it can never get breaking changes; they just have to come as part of a semver-major, which happens annually. So maybe we shouldn’t be scared off by perpetually experimental specs and just go ahead and mark our features as stable, and if the spec changes then we’ll ship those changes in the next major.

In the WASI case if the constructor can be used as a way to essentially request a particular spec version, that sounds like a great solution that I wish I had for import assertions, and WASI going stable seems especially low risk.

@mcollina
Copy link
Member

I'm +1 for marking it stable. I'm also ok in unflagging it and keep it experimental with or without a warning.

@GeoffreyBooth
Copy link
Member

Okay, so some proposed next steps:

  • A PR to add the version option to the constructor, and probably make it required. Land this and then let some time go by for folks to try it out and report issues.
  • A PR to make WASI stable and drop the flag/warning. This PR should also update the docs to include some text that the WASI spec is itself in flux, and that changes to the spec that would result in breaking changes to Node will arrive in semver-major updates in Node. Hopefully the new constructor will allow most spec changes to land without them needing to be breaking.

I think that’s it?

@mhdawson
Copy link
Member

I'm +1 for marking it stable. I'm also ok in unflagging it and keep it experimental with or without a warning.

+1 from me on that as well

@phated
Copy link
Contributor

phated commented Jan 29, 2023

Having uvwasi be actively maintained is something @ospencer and I are really interested in, both for nodejs and WAMR support (they currently have it marked as experimental too). Please let us know how we can help.

@mhdawson
Copy link
Member

@ospencer, @phated if you two have time to contribute I can set up a meeting with myself and @cjihrig to discuss how to get started and some specific work to get started on. Does that make sense?

mhdawson added a commit to mhdawson/io.js that referenced this issue Feb 1, 2023
Refs: nodejs#46254

- add version to options when creating WASI object
- add convenience function to return importObject

Signed-off-by: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

mhdawson commented Feb 1, 2023

Link for doodle to discuss contributing to uvwasi - https://doodle.com/meeting/participate/id/e36JQMOd
@ospencer, @phated, @cjihrig could you fill in your availability?

If there are others interested in contributing to uvwasi as well please feel to join as well.

@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 1, 2023

@mhdawson sorry to be an inconvenience, but I'll be traveling next week. Could we aim for the week after?

@mhdawson
Copy link
Member

mhdawson commented Feb 1, 2023

@cjihrig ack doodle updated to be for times the week after - https://doodle.com/meeting/participate/id/e36JQMOd

@ospencer, @phated if you already voted please look and vote again.

@ospencer
Copy link
Member

ospencer commented Feb 1, 2023

All set; thanks for organizing @mhdawson!

@phated
Copy link
Contributor

phated commented Feb 3, 2023

Ditto!

@mhdawson
Copy link
Member

mhdawson commented Feb 6, 2023

@ospencer, @phated, @cjihrig thanks for completing the doodle, have sent out an invite for Feb 15 from 2-3pm ET

@tniessen
Copy link
Member

@cjihrig @mhdawson Sorry for jumping into this conversation this late, but I was wondering if JSPI will affect our WASI implementation at all. In particular, should wasi.start() return a Promise?

mhdawson added a commit that referenced this issue Feb 22, 2023
Refs: #46254

- add version to options when creating WASI object
- add convenience function to return importObject

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #46469
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@toyobayashi
Copy link
Contributor

I have a question about Node.js WASI API when I use it in Worker to initialize a wasm module compiled by wasi-sdk-20+threads (wasm32-wasi-threads target), it needs imported shared memory from main thread. And calling instance.exports._initialize with shared memory in child thread will throw error.

nodejs/help#4102

@mhdawson
Copy link
Member

mhdawson commented Mar 2, 2023

@tniessen in terms of

Sorry for jumping into this conversation this late, but I was wondering if JSPI will affect our WASI implementation at all. In particular, should wasi.start() return a Promise?

I don't think wasi.start() is blocking in the sense of it waiting until the WebAssembly execution is complete. @cjihrig do I have that wrong?

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 2, 2023

I think @tniessen is correct. Right now, start() kicks off the WASM execution, and we use a hack based on try...catch to allow the WASM code to "exit" the process without actually exiting the Node process. All of that happens synchronously because everything on the WASM side is synchronous up to this point.

It probably makes sense to make start() asynchronous. That said, I also haven't followed along with the snapshot2 work closely enough to know if start() and initialize() will even make sense moving forward.

targos pushed a commit that referenced this issue Mar 13, 2023
Refs: #46254

- add version to options when creating WASI object
- add convenience function to return importObject

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #46469
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@mhdawson
Copy link
Member

I've heard that there is a suggestion that we cut off all changes on April 1 so we may need to make a decision in the next week or so. @cjihrig

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 22, 2023

After the last few Node WASI team meetings we've had, my personal opinion is that we should not mark things as stable at this time. I'm not going to block it if other people feel differently though. I also have limited availability this week, so if something needs to happen quickly, you don't need to wait to hear back from me.

@mcollina
Copy link
Member

As an intermediate step, could we drop the flag and emit a warning on use instead?

@tniessen
Copy link
Member

During the last uvwasi meeting, I suggested as an intermediate goal to mark node:wasi as stable while keeping the actual implementations of the different WASI versions experimental. This is similar to how Web Crypto itself is stable, but some Web Crypto algorithms are not. However, for that to happen, we have to carefully resolve API design issues such as #46254 (comment) first, so I don't see it happening in time for Node.js 20.

@GeoffreyBooth
Copy link
Member

mark node:wasi as stable while keeping the actual implementations of the different WASI versions experimental

This seems like a good opportunity to start labeling things using the more detailed experimental stages outlined in #46100. That PR hasn't been released yet (it's just in main) though.

@tniessen
Copy link
Member

This seems like a good opportunity to start labeling things using the more detailed experimental stages

IIRC it was decided that these hypothetical experimental stages should not correspond to flags/warnings, so I am not sure how that helps. What I meant is that only some features could be marked as experimental, not the entire module. See the Web Crypto: Algorithm matrix, for example.

@mhdawson
Copy link
Member

As an intermediate step, could we drop the flag and emit a warning on use instead?

This seems like an resonable next step. If people agree that is a good idea I'd be willing to put together a PR.

@mhdawson
Copy link
Member

PR to remove need for command line flag - #47286. WASI is still documented as experimental.

@JamieMagee
Copy link

It looks like the linked PR was merged, and I can't see --experimental-wasi-unstable-preview1 in recent nightlies. Can this issue be closed?

@tniessen
Copy link
Member

No, WASI remains experimental until the API is finalized.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Oct 14, 2023
@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 2, 2023

It's worth noting that Deno has deprecated their WASI implementation "due to low usage and minimal feedback from the community."

@github-actions github-actions bot removed the stale label Nov 3, 2023
@d3lm
Copy link
Contributor

d3lm commented Nov 10, 2023

We added WASI support to WebContainers (same code as Node.js) now and it's really essential for running native code and already powers our experimental Python support. Wanted to add an extra data point. I think WASI is going to get some traction as it stablized and it hits 1.0. WASI is already used quite a bunch on the Edge, e.g. Cloudflare has announced WASI support for Cloudflare Workers so I'd personally not like to see WASI being deprecated for Node.js.

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 10, 2023

I'm not sure what you mean by "same code as Node.js" but you could implement WASI as a compiled addon or try something like jco.

There is a pretty big difference between what WASI was in snapshot 1 and its direction now, and I don't believe it makes sense to have it in core anymore (I'm not sure of any JS runtime working on anything beyond snapshot 1). We also have the problem of maintaining WASI in core. There is such little interest in maintaining it that the project recently marked its WASI implementation as insecure rather than fix a small to medium sized bug.

@guybedford
Copy link
Contributor

guybedford commented Nov 14, 2023

To try to fill in some of the blanks here on stability:

  • WASI Preview1 is a stable specification, it will not be undergoing major changes going forward. As per the first point in this issue, the WASI version is now part of the public API so that new WASI({ version: 'preview1' }) can laregely be considered stable at this point in Node.js.
  • The security issue @cjihrig is referring to requires a fundamental change to libuv and the entire pathing system. While it isn't a huge effort given a fair amount of development time, it also isn't quite that trivial. See Discussion: openat support libuv/libuv#4167 for more info here.
  • The WASI security model has now been formally declared unsupported and rather a future feature in the documentation - so that adding this security property is a backwards compatible feature addition and doesn't affect the stability.
  • I would therefore argue that API stability is a function of if we want to make any breaking changes for Preview 2 support. We have a path to preview 2 support that could be non-breaking, but there could be benefits in some slight tweaks to the API shapes as well.

I'm actively working on the preview2 support in userland via the jco project for Node.js currently, and hope to use this implementation experience to feed back some suggestions for preview2 support in core. Having support in core will allow optimization opportunities not otherwise possible in userland, so I think that would be very worthwhile and I'm actively interested in working on these directions myself.

If anyone else is interested in working on WASI from the security properties to preview2 support designs, the @nodejs/wasi team is active and has meetings every two weeks, with the next on 22 November. Agendas for the meetings are posted in https://github.com/nodejs/uvwasi/issues a few days before.

@d3lm
Copy link
Contributor

d3lm commented Nov 15, 2023

@guybedford I would be interested in working on WASI because we have interest in this at StackBlitz and I think it'd be really nice if we could keep WASI in the core of Node.js. It just allows for a handful of optimizations, as Guy pointed out, that wouldn't be possible otherwise. Furthermore, I see it as another battery included in Node.js and from a DX perspective it's much nicer if you didn't have to rely on a separate project.

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 15, 2023

Having support in core will allow optimization opportunities not otherwise possible in userland

I'm genuinely curious what the optimizations are.

requires a fundamental change to libuv and the entire pathing system

Technically, no changes to libuv are required. The openat() functionality could be added to Node.js or uvwasi. I'm assuming uvwasi is not an option with jco though.

I also have a few questions:

  • Which WASI worlds will be supported? Other than maintenance of WASI in Node, this is my biggest issue with continuing to include it in core. I don't think supporting only a single world in core makes a lot of sense.
  • My (limited) understanding of jco is that it will implement WASI in terms of JavaScript calls. Has any benchmarking been done to see how the performance compares to the current solution in native code?

@guybedford
Copy link
Contributor

Which WASI worlds will be supported?

Certainly wasi-cli along with its associated worlds. Supporting wasi-http would be great to see as well.

My (limited) understanding of jco is that it will implement WASI in terms of JavaScript calls.

There's high-level and low-level bindgen, and we are doing high-level bindgen currently in the name of correctness and because it should always be supported to directly call to WASI from JS (since that is what high-level bindgen is). Anything upstreamed into Node.js would be based on performance-optimized low-level bindgen that uses the direct core component model ABIs. We might need something like a WASI.instantiate to properly encapsulate this process, although Node.js should also support the high-level bindings too, so that it may be possible without and just having a property on the high-level function pointing to the low-level implementation.

That would then be somewhat analogous to WebAssembly function imports, where the JS function wrapper is a high-level JS function that is passed from JS into Wasm instantiation, but when passing that import to a Wasm core module, it instead reads out and attaches to the direct low-level function not the high-level JS function wrapper.

@cjihrig cjihrig closed this as completed Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. wasi Issues and PRs related to the WebAssembly System Interface.
Projects
None yet
Development

No branches or pull requests