Conversation
c8be8a4 to
d1db043
Compare
|
|
||
| func-item ::= id ':' func-type ';' | ||
|
|
||
| func-type ::= 'func' param-list result-list |
There was a problem hiding this comment.
Could you add some text to Wit.md briefly describing what non-blocking does, from a Wit user perspective? And maybe link back to the Explainer.md for the full details?
design/mvp/Async.md
Outdated
| ``` | ||
| If a resource-type has a potentially-blocking constructor, it can simply use | ||
| `static new: func(...) -> my-resource` instead; `constructor` has no advantages | ||
| beyond more-idiomatic bindings generation in some languages. |
There was a problem hiding this comment.
From a new Wit user perspective. the name "non-blocking" could sound like it means "no stopping the caller" rather than "no waiting for I/O", which would be confusing since it does the exact opposite of that :-}. At least we should clearly document the Wit-user-facing side of this keyword.
There was a problem hiding this comment.
Yeah, I suppose "blocking" does have multiple interpretations. I added this addition to the previous note, but it could probably be improved.
design/mvp/Explainer.md
Outdated
| a `valtype`. | ||
| a `valtype`. Function types can optionally be annotated with a `non-blocking` | ||
| attribute which has no semantic effect and is ignored at validation- and | ||
| run-time, serving primarily as a hint that tells bindings generators to lift |
There was a problem hiding this comment.
Is the "primarily" here redundant?
There was a problem hiding this comment.
Yeah, I suppose so. It felt a bit off without any word, so I tried "only", and also tightened up the rest of the sentence in this commit, PTAL
3bdea27 to
c81fbb2
Compare
|
Since this is just a hint for the binding generators and not actually enforced at runtime: as a WIT author I'd rather prefer the defaults to be swapped. I.e. functions should be generated as As an example I annotated the wasi-sockets proposal with both defaults:
Aside from the amount of work it is for me (which I can live with :P ), annotating only the "blocking" functions is more familiar to readers who have experience with |
I said the same thing when @lukewagner and I discussed this a while ago. I believe his response was that constructors are non-blocking by convention and that, once WIT has special syntax for resource getters and setters, those will also be non-blocking by convention, which should reduce the number of explicit annotations required. That said, I still tend to agree with you about swapping, since experience has show that functions in the average WIT interface that involve I/O are in the minority, even after you ignore the constructors, getters, and setters. |
|
Scanning that wasi-sockets PR, 28 of the 34 For the case of For the case of But for the last case, |
|
Regarding As for
I see your point (and the "but not hard" caveat), but I don't know how to extrapolate that guidance:
Having native syntax for getters & setters could indeed help alleviate many of the annotations in wasi-sockets. As you already counted out, that would bring the amount of needed annotations down from 34 to just 6. Versus 5. So: potáto, potàto. 🙃 My other point still stands though; this is a syntax is directed at binding generators / language implementors. IMO it still makes sense to speak "their" language, not "ours". From a JavaScript/Python/Rust/C#/.. developer's POV, seeing an |
|
@badeend Good points! Revisiting the case of Happy to hear more thoughts on this! |
|
Polling more folks during the BA component-model implementation meeting, lots of agreement on switching the default. But the meeting also highlighted that @sunfishcode was right above in saying that "blocking" can naturally be interpreted to have the opposite meaning that I'm assuming in the PR. Thinking of what a good short intuitive alternative keyword is, Thus, in summary, it seems like a good idea to default to synchronous bindings with an explicit opt-in via |
|
Virtualization adds an interesting perspective to this, because people may sometimes make APIs that are fine to be synchronous initially, but which other people later want to virtualize using slower implementations. A historic example of this is Unix thinking that disks are fast and making all filesystem APIs synchronous. Then when they tried to "virtualize" it with NFS it had to try to fake synchronous semantics while being async under the covers, which got awkward. Also, "async" may give people the wrong intuition with respect to wasm's colorless async composition. Sync code can still call these "async" functions, using sync bindings. I'm fond of the idea of making "blocking" (aka "async") the default, provided we can find a sufficiently intuitive keyword for "non-blocking". How does "shallow" sound? bind: shallow func(local-address: ip-socket-address) -> result<_, error-code>;"Shallow" evokes the idea that this function doesn't have a deep call tree; it just configures a local data structure and then returns. Perhaps not obvious enough to intuit from first principles, but might feel natural once learned. |
|
I also like the idea of "async by default", but the monkey wrench for me was cases like Also, while I also worry about |
|
Given that the new opt-in keyword ( I was enthusiastic about an async ABI default as well, but it clearly seems the less frequent choice once |
Ah, that's true. Initially I was thinking that the alternative would be to just have bindings generators default to sync bindings. That would also eliminate the same surprises. But the place where this breaks is if you have a mix of p2 and p3 APIs, and you want to use async bindings for the p3 APIs, but that would make all your p2 APIs use async bindings too, which they likely weren't designed for. In other words, we have this table:
Which shows how if we made
I don't think foo: blocking func() -> s32;
foo: non-blocking func() -> futue<s32>;The |
|
And following up on my comment here, since it makes sense for the default to be non-blocking, "shallow" is the wrong sense, and I agree that it makes sense to call the keyword "async" for familiarity. |
5920c97 to
af420d8
Compare
|
Ok, I updated the PR to flip defaults and use Working on this, I realized that there is a much better place to put a hint that doesn't affect static or dynamic semantics and only informs bindgen: in the import/export name string. Thus, I updated the PR to encode the |
|
From implementing this: Today we disallow importing both
|
This commit is an implementation of WebAssembly/component-model#442 in this repository, namely: * The `async` identifier is now a keyword in WIT. * Functions in WIT can be annotated `async`. * New kebab-names are now recognized to round-trip this attribute through the wasm binary format: * `[async]foo` => `foo: async func()` * `[async method]foo.bar` => `resource foo { bar: async func() }` * `[async static]foo.bar` => `resource foo { bar: static async func() }` * Various new tests were updated to ensure these names are gated behind the component-model-async feature and additionally test some validation of these names. * The `wit-component` convention of "name mangling" was updated to use `[async-lower]` instead of `[async]` for lowered functions (to not conflict with the kebab-name otherwise. Additionally in exports `[async-lift]` is used instead of `[async]` as well as `[async-lift-stackful]` instead of `[async-stackful]`. * Support was added to `wit-smith` to generate `async` functions.
|
Ah, great point, I forgot about that, and those restrictions make sense. What if we took it one small step further and said simply "names must be unique within a scope after stripping any |
That seems like the safest option to me. 👍 That would also automatically work for any new annotation (e.g. |
|
I think that rule might run afoul of making this currently valid component invalid though? (component
(import "a" (type $a (sub resource)))
(import "[constructor]a" (func (result (own $a))))
) |
|
Ah right, thanks; I forgot about the type import. Could we say "names must be unique in a scope after stripping |
|
Sounds reasonable to me 👍 |
af420d8 to
6967620
Compare
|
Ok, rebased to avoid conflicts, and I added this commit to cover naming uniqueness conditions. This commit factors out several scattered bits of uniqueness requirements into a new section in Explainer.md that defines the term "strongly-unique" that attempts to cover what we just discussed. PTAL! |
This commit is an implementation of WebAssembly/component-model#442 in this repository, namely: * The `async` identifier is now a keyword in WIT. * Functions in WIT can be annotated `async`. * New kebab-names are now recognized to round-trip this attribute through the wasm binary format: * `[async]foo` => `foo: async func()` * `[async method]foo.bar` => `resource foo { bar: async func() }` * `[async static]foo.bar` => `resource foo { bar: static async func() }` * Various new tests were updated to ensure these names are gated behind the component-model-async feature and additionally test some validation of these names. * The `wit-component` convention of "name mangling" was updated to use `[async-lower]` instead of `[async]` for lowered functions (to not conflict with the kebab-name otherwise. Additionally in exports `[async-lift]` is used instead of `[async]` as well as `[async-lift-stackful]` instead of `[async-stackful]`. * Support was added to `wit-smith` to generate `async` functions.
* Force using `sync` resource-drop during fuzzing This commit fixes fuzzer fallout from #2065 and related PRs where previously async destructor imports were being generated but were actually bound with synchronous destructor imports. Now `wit-component` is generating an error on async resource destructors so this fixes the generator to avoid generating such imports for now while there's not support for lowering them. * Implement the WIT `async` keyword This commit is an implementation of WebAssembly/component-model#442 in this repository, namely: * The `async` identifier is now a keyword in WIT. * Functions in WIT can be annotated `async`. * New kebab-names are now recognized to round-trip this attribute through the wasm binary format: * `[async]foo` => `foo: async func()` * `[async method]foo.bar` => `resource foo { bar: async func() }` * `[async static]foo.bar` => `resource foo { bar: static async func() }` * Various new tests were updated to ensure these names are gated behind the component-model-async feature and additionally test some validation of these names. * The `wit-component` convention of "name mangling" was updated to use `[async-lower]` instead of `[async]` for lowered functions (to not conflict with the kebab-name otherwise. Additionally in exports `[async-lift]` is used instead of `[async]` as well as `[async-lift-stackful]` instead of `[async-stackful]`. * Support was added to `wit-smith` to generate `async` functions. * Fix `Ord for ComponentNameKind` * Another `Ord`-related fix
* Force using `sync` resource-drop during fuzzing This commit fixes fuzzer fallout from bytecodealliance#2065 and related PRs where previously async destructor imports were being generated but were actually bound with synchronous destructor imports. Now `wit-component` is generating an error on async resource destructors so this fixes the generator to avoid generating such imports for now while there's not support for lowering them. * Implement the WIT `async` keyword This commit is an implementation of WebAssembly/component-model#442 in this repository, namely: * The `async` identifier is now a keyword in WIT. * Functions in WIT can be annotated `async`. * New kebab-names are now recognized to round-trip this attribute through the wasm binary format: * `[async]foo` => `foo: async func()` * `[async method]foo.bar` => `resource foo { bar: async func() }` * `[async static]foo.bar` => `resource foo { bar: static async func() }` * Various new tests were updated to ensure these names are gated behind the component-model-async feature and additionally test some validation of these names. * The `wit-component` convention of "name mangling" was updated to use `[async-lower]` instead of `[async]` for lowered functions (to not conflict with the kebab-name otherwise. Additionally in exports `[async-lift]` is used instead of `[async]` as well as `[async-lift-stackful]` instead of `[async-stackful]`. * Support was added to `wit-smith` to generate `async` functions. * Fix `Ord for ComponentNameKind` * Another `Ord`-related fix
This PR adds
non-blockingtofunctype(both in WAT and WIT). It's a pretty small addition and doesn't touch validation/runtime.While it initially seemed attractive to enforce
non-blockingwith trap-on-block semantics, there are valid scenarios where a callee might actually want/need to block and where the loss of concurrency in the caller is fine. Thus the PR proposes makingnon-blockingjust a hint (ignored by validation/runtime) to inform bindings generation (e.g., allowing bindings generators that make all functionsasyncby default emit non-asyncfunctions fornon-blocking).constructorimpliesnon-blocking(sincenewexpressions in most languages can't be async). However, to avoid breaking wasip2, we don't (yet) require validation of[constructor]-named functions to containnon-blocking(adding this to the list of warts to remove in the next breaking change). In any case, bindings generators can always just take the non-blocking hint directly from seeing[constructor].