Stream: git-wasmtime

Topic: wasmtime / PR #3901 feat(wasi)!: clean up `WasiFile` impl...

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 15:05):

npmccallum opened PR #3901 from wasi to main:

This effort includes two major changes:

  1. Most methods are given a default implementation. This aids
    implementors who only need a few of the methods and it allows us to
    remove a bunch of boilerplate code.

  2. Most methods are changed to take a mutable reference. This is
    important since wasmtime cannot know ahead of time what APIs
    implementors are trying to adapt.

BREAKING CHANGE: WasiFile has changed method signatures

Signed-off-by: Nathaniel McCallum <nathaniel@profian.com>

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 15:22):

npmccallum updated PR #3901 from wasi to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 15:39):

npmccallum updated PR #3901 from wasi to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 15:49):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 15:49):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 15:49):

bjorn3 created PR review comment:

An argument against this change: Using &mut self would require locking on the wasmtime side if multi threading support is ever implemented. &self allows the WasiFile impl to avoid locking if it doesn't need it. (eg you can read from and write to an &std::fs::File without locking. &mut self would also require double locking in case of a shared resource accessible through multiple WasiFiles.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 15:49):

bjorn3 created PR review comment:

I think the intention here is that it will eventually be implemented once it becomes possible.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 16:10):

npmccallum submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 16:10):

npmccallum created PR review comment:

@bjorn3 Except that this is an async interface. I'm unware of async-aware locking primitives. And you most certainly don't want to take a lock around an IO operation.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 16:31):

npmccallum submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 16:31):

npmccallum created PR review comment:

@bjorn3 The existing code is inconsistent with regards to mutability. Whatever we do, it should be consistent.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 16:46):

npmccallum submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 16:46):

npmccallum created PR review comment:

@bjorn3 Also, WasiSnapshotPreview1 takes &mut self for all operations. That seems to gate this.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 17:07):

npmccallum updated PR #3901 from wasi to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 17:11):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 17:11):

bjorn3 created PR review comment:

I guess so.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 22:44):

npmccallum updated PR #3901 from wasi to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 23:14):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 23:22):

pchickey merged PR #3901.

Last updated: Nov 22 2024 at 16:03 UTC