Stream: git-wasmtime

Topic: wasmtime / PR #7417 Turn `request-options` into a resource

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:39):

elliottt requested alexcrichton for a review on PR #7417.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:39):

elliottt requested wasmtime-core-reviewers for a review on PR #7417.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:39):

elliottt opened PR #7417 from elliottt:trevor/request-options-resource to bytecodealliance:main:

As we'd like to be able to modify request-options in the future without breaking compatibility with older guests, turn request-options into a resource. This allows us to expose getters and setters instead of fields on a record, which gives the embedder flexibility in what additional options are allowed.
<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:39):

elliottt edited PR #7417:

As we'd like to be able to modify request-options in the future without breaking compatibility with older guests, turn request-options into a resource. This allows us to expose getters and setters instead of fields on a record, which gives the embedder flexibility in what additional options are allowed.

Fixes #7409
<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:40):

elliottt requested pchickey for a review on PR #7417.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:43):

alexcrichton submitted PR review:

Looks good to me.

Random drive-by thought unrelated to this change specifically, could the *-ms bits be updated to take the duration type in https://github.com/bytecodealliance/wasmtime/pull/7358 which would give more precision here as well?

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:44):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:44):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:44):

pchickey created PR review comment:

Lets use either std or tokio::time::Duration to represent these values in the host as early as we can.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:44):

pchickey created PR review comment:

Each of these setters needs to return result, where the error means that the setting is not supported.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:49):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:49):

elliottt created PR review comment:

The downside is that the getters become quite fallible as they're turning a std::time::Duration back into a u32.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:49):

elliottt updated PR #7417.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:51):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:51):

pchickey created PR review comment:

Isnt that resolved when we switch to using a monotonic duration, which is a u64?

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:54):

elliottt updated PR #7417.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:55):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 30 2023 at 21:55):

elliottt created PR review comment:

std::time::Duration::as_millis returns a u128, so we won't avoid it completely.

Wasmtime GitHub notifications bot (Oct 30 2023 at 22:00):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 30 2023 at 22:00):

elliottt created PR review comment:

I implemented this by treating conversion failure from u128 as an Ok(None) return value. Since we default all these timeouts to None anyway, I think a conversion failure would mean that the guest somehow managed to set a larger than u32 value to begin with, then tried to get one back. The host setting an invalid default would be the other possibility, but we don't do this anywhere in wasmtime currently.

Wasmtime GitHub notifications bot (Oct 30 2023 at 22:49):

elliottt updated PR #7417.

Wasmtime GitHub notifications bot (Oct 30 2023 at 23:40):

elliottt updated PR #7417.

Wasmtime GitHub notifications bot (Oct 30 2023 at 23:55):

elliottt has enabled auto merge for PR #7417.

Wasmtime GitHub notifications bot (Oct 31 2023 at 00:49):

elliottt merged PR #7417.

Last updated: Oct 23 2024 at 20:03 UTC