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

Generate async drop methods for resources. #9091

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

badeend
Copy link
Contributor

@badeend badeend commented Aug 8, 2024

Add the ability to generate async drop methods for resources.

In the component model, resource.drop is a canonical built-in without a proper name. So I invented a custom naming scheme for the component bindgen config. I went with: "[drop]{resource-name}" where {resource-name} is the name as defined in WIT. e.g. "[drop]input-stream".

This shouldn't conflict with anything existing in the wild as WIT identifiers are not allowed to contain square brackets.


To the reviewer: please double check resource_async. I'm not familiar with this part of the codebase.


Spawned off from #9058. This change is a prerequisite for a potential solution of that issue, but seems useful enough on its own.

@badeend badeend requested a review from a team as a code owner August 8, 2024 09:20
@badeend badeend requested review from pchickey and removed request for a team August 8, 2024 09:20
In the component model, `resource.drop` is a canonical built-in without a proper name. So I invented a custom naming scheme for the component bindgen config. I went with:
`"[drop]{resource-name}"` where `{resource-name}` is the name as defined in WIT. e.g. `"[drop]input-stream"`.

This shouldn't conflict with anything existing in the wild as WIT identifiers are not allowed to contain square brackets.
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Aug 8, 2024
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me, thanks! Would you be up for adding a test for this as well?

@badeend
Copy link
Contributor Author

badeend commented Aug 12, 2024

If you're looking for something to test this feature in isolation I'd be glad to add a test for it, but could you point me towards where the component model tests live currently? I tried looking for them in the usual places but couldn't find them.

If all you're after is that this is tested somehow and don't care about the test being in isolation or not, I've got a follow-up PR planned for #9058 that will be using this feature. So it will be tested indirectly through that.

@alexcrichton
Copy link
Member

Yes I think that this would be good to have a dedicated test for just this. Component-model API tests live at https://github.com/bytecodealliance/wasmtime/tree/main/tests/all/component_model and https://github.com/bytecodealliance/wasmtime/blob/main/tests/all/component_model/async.rs has some examples of doing async things

@badeend
Copy link
Contributor Author

badeend commented Aug 14, 2024

Check!

@alexcrichton alexcrichton added this pull request to the merge queue Aug 14, 2024
Merged via the queue into bytecodealliance:main with commit c1663ba Aug 14, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants