Stream: git-wasmtime

Topic: wasmtime / PR #5326 feat: remove mutability from the Was...

Wasmtime GitHub notifications bot (Nov 30 2022 at 09:21):

haraldh edited PR #5326 from table_immutable to main:

This patch adds interior mutability to the WasiCtx Table and the Table elements.

Major pain points:

• File only needs RwLock<cap_std::fs::File> to implement File::set_fdflags() on Windows, because of https://github.com/bytecodealliance/system-interface/blob/da238e324e752033f315f09c082ad9ce35d42696/src/fs/fd_flags.rs#L210-L217
• Because File needs a RwLock and RwLock*Guard cannot be hold across an .await, The async from async fn num_ready_bytes(&self) had to be removed
• Because File needs a RwLock and RwLock*Guard cannot be dereferenced in pollable, the signature of fn pollable(&self) -> Option<rustix::fd::BorrowedFd> changed to fn pollable(&self) -> Option<Arc<dyn AsFd + '_>>

Wasmtime GitHub notifications bot (Nov 30 2022 at 09:21):

haraldh edited PR #5326 from table_immutable to main:

This patch adds interior mutability to the WasiCtx Table and the Table elements.

Major pain points:

• File only needs RwLock<cap_std::fs::File> to implement File::set_fdflags() on Windows, because of https://github.com/bytecodealliance/system-interface/blob/da238e324e752033f315f09c082ad9ce35d42696/src/fs/fd_flags.rs#L210-L217
• Because File needs a RwLock and RwLock*Guard cannot be hold across an .await, The async from async fn num_ready_bytes(&self) had to be removed
• Because File needs a RwLock and RwLock*Guard cannot be dereferenced in pollable, the signature of fn pollable(&self) -> Option<rustix::fd::BorrowedFd> changed to fn pollable(&self) -> Option<Arc<dyn AsFd + '_>>

Related: https://github.com/bytecodealliance/wasmtime/issues/5235

Wasmtime GitHub notifications bot (Nov 30 2022 at 16:22):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2022 at 16:22):

alexcrichton created PR review comment:

Hm ok sorry I'm having trouble phrasing my question/concern apparently. Let me try again. I do not understand the relevance to the #[cfg(windows)] block you mentioned above. I don't know what that function is, why it exists, why it's Windows-specific, or why it's at all related to wasi-common and this PR. What in WASI supports reopening a file with new flag and modifying a handle?

I don't understand these connections so I'm under the impression that the rwlock shouldn't be needed here. Can you help me understand the connections here?

Wasmtime GitHub notifications bot (Dec 01 2022 at 09:13):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Dec 01 2022 at 09:13):

haraldh created PR review comment:

wasmtime/crates/wasi-common/cap-std-sync/src/file.src

impl WasiFile for File {
    // ...
    async fn set_fdflags(&self, fdflags: FdFlags) -> Result<(), Error> {
        if fdflags.intersects(
            wasi_common::file::FdFlags::DSYNC
                | wasi_common::file::FdFlags::SYNC
                | wasi_common::file::FdFlags::RSYNC,
        ) {
            return Err(Error::invalid_argument().context("cannot set DSYNC, SYNC, or RSYNC flag"));
        }
        let mut file = self.0.write().unwrap();
        let set_fd_flags = (*file).new_set_fd_flags(to_sysif_fdflags(fdflags))?;
        (*file).set_fd_flags(set_fd_flags)?;
        Ok(())
    }
    // ...
}

This is the only function which needs the RwLock::write() (&mut self before) of the whole impl WasiFile for File. It calls out to cap_std::fs::File::set_fd_flags(), which wants &mut self`.

cap_std::fs::File automatically implements the GetSetFdFlags of the system-interface crate, which provides the set_fd_flags() method, to which I linked above.

Apparently the only way to change the "fd" flags on windows, is to reopen the file, which will get you another Handle. So the way it is implemented, it changes the inner handle for the File handle.

        *self = set_fd_flags.reopened;

This is why self has to be mutable for set_fd_flags. And that is only the case on Windows.

Wasmtime GitHub notifications bot (Dec 01 2022 at 09:13):

haraldh edited PR review comment.

Wasmtime GitHub notifications bot (Dec 01 2022 at 09:14):

haraldh edited PR review comment.

Wasmtime GitHub notifications bot (Dec 01 2022 at 19:44):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 01 2022 at 19:44):

alexcrichton created PR review comment:

Ok that makes sense, thanks for explaning. Personally I feel like this has a significant impact enough that I think it should be solved before merging.

Digging in further, this feels like a bit of an odd API. All of the *SYNC flags aren't supported on Unix but they are sort of supported on Windows so there's already precedent for platform-specific behavior, although I'm not sure if that's desired or not. Additionally Windows rejects the NONBLOCK flag which means the only thing Windows supports here is APPEND. Forcing the entire API to have a lock unnecessarily for all platforms just for this one use case feels a bit off to me.

I would propose a course of action going forward entailing:

• Rethinking this API for WASI and whether it makes sense (cc @sunfishcode), for example changing the API could remove the need for this implementation
• In the meantime, change the trait signature to self: &Arc<Self> or something like that and use Arc::get_mut to keep this working in single-threaded situations while otherwise just returning an error in multi-threaded situations.

I think that should remove the need for the RwLock/poll changes, set this up for having a real solution in the long run, and getting something reasonable working for now.

Wasmtime GitHub notifications bot (Dec 02 2022 at 00:48):

bstrie submitted PR review.

Wasmtime GitHub notifications bot (Dec 02 2022 at 00:48):

bstrie created PR review comment:

I'm happy to adapt this PR as described above. I agree that it's better to have something reasonable that we can iterate on in the short term.

Wasmtime GitHub notifications bot (Dec 20 2022 at 17:55):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 17:55):

abrown created PR review comment:

I took a look at @alexcrichton's suggestion yesterday and abandoned after fixing compiler errors for several hours. The switch to Arc<Self> affects many places and after fixing up many of these places I was not sure if this approach would even work in the end. Here is the WIP commit, but note that not all errors are fixed at this point (probably 20+ left): https://github.com/abrown/wasmtime/compare/pr-5326...abrown:wasmtime:pr-5326-arc. @alexcrichton can you take a look?

Wasmtime GitHub notifications bot (Jan 03 2023 at 16:41):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 03 2023 at 16:41):

alexcrichton created PR review comment:

Ah sorry I meant moreso self: &Arc<Self> on just that one method. Trying that myself runs afoul of object safety issues, so the alternative is:

<details>

diff --git a/crates/wasi-common/cap-std-sync/src/file.rs b/crates/wasi-common/cap-std-sync/src/file.rs
index c54a5ead7..c184486a7 100644
--- a/crates/wasi-common/cap-std-sync/src/file.rs
+++ b/crates/wasi-common/cap-std-sync/src/file.rs
@@ -93,7 +93,7 @@ impl WasiFile for File {
         let fdflags = get_fd_flags(&*file)?;
         Ok(fdflags)
     }
-    async fn set_fdflags(&self, fdflags: FdFlags) -> Result<(), Error> {
+    async fn set_fdflags(&mut self, fdflags: FdFlags) -> Result<(), Error> {
         if fdflags.intersects(
             wasi_common::file::FdFlags::DSYNC
                 | wasi_common::file::FdFlags::SYNC
@@ -101,7 +101,7 @@ impl WasiFile for File {
         ) {
             return Err(Error::invalid_argument().context("cannot set DSYNC, SYNC, or RSYNC flag"));
         }
-        let mut file = self.0.write().unwrap();
+        let file = self.0.get_mut().unwrap();
         let set_fd_flags = (*file).new_set_fd_flags(to_sysif_fdflags(fdflags))?;
         (*file).set_fd_flags(set_fd_flags)?;
         Ok(())
diff --git a/crates/wasi-common/cap-std-sync/src/net.rs b/crates/wasi-common/cap-std-sync/src/net.rs
index b46d8d7da..921622d68 100644
--- a/crates/wasi-common/cap-std-sync/src/net.rs
+++ b/crates/wasi-common/cap-std-sync/src/net.rs
@@ -98,7 +98,7 @@ macro_rules! wasi_listen_write_impl {
             }
             async fn sock_accept(&self, fdflags: FdFlags) -> Result<Box<dyn WasiFile>, Error> {
                 let (stream, _) = self.0.accept()?;
-                let stream = <$stream>::from_cap_std(stream);
+                let mut stream = <$stream>::from_cap_std(stream);
                 stream.set_fdflags(fdflags).await?;
                 Ok(Box::new(stream))
             }
@@ -110,7 +110,7 @@ macro_rules! wasi_listen_write_impl {
                 let fdflags = get_fd_flags(&self.0)?;
                 Ok(fdflags)
             }
-            async fn set_fdflags(&self, fdflags: FdFlags) -> Result<(), Error> {
+            async fn set_fdflags(&mut self, fdflags: FdFlags) -> Result<(), Error> {
                 if fdflags == wasi_common::file::FdFlags::NONBLOCK {
                     self.0.set_nonblocking(true)?;
                 } else if fdflags.is_empty() {
@@ -197,7 +197,7 @@ macro_rules! wasi_stream_write_impl {
                 let fdflags = get_fd_flags(&self.0)?;
                 Ok(fdflags)
             }
-            async fn set_fdflags(&self, fdflags: FdFlags) -> Result<(), Error> {
+            async fn set_fdflags(&mut self, fdflags: FdFlags) -> Result<(), Error> {
                 if fdflags == wasi_common::file::FdFlags::NONBLOCK {
                     self.0.set_nonblocking(true)?;
                 } else if fdflags.is_empty() {
diff --git a/crates/wasi-common/src/file.rs b/crates/wasi-common/src/file.rs
index b76278373..828307255 100644
--- a/crates/wasi-common/src/file.rs
+++ b/crates/wasi-common/src/file.rs
@@ -64,7 +64,7 @@ pub trait WasiFile: Send + Sync {
         Ok(FdFlags::empty())
     }

-    async fn set_fdflags(&self, _flags: FdFlags) -> Result<(), Error> {
+    async fn set_fdflags(&mut self, _flags: FdFlags) -> Result<(), Error> {
         Err(Error::badf())
     }

@@ -217,11 +217,15 @@ pub struct Filestat {

 pub(crate) trait TableFileExt {
     fn get_file(&self, fd: u32) -> Result<Arc<FileEntry>, Error>;
+    fn get_file_mut(&mut self, fd: u32) -> Result<&mut FileEntry, Error>;
 }
 impl TableFileExt for crate::table::Table {
     fn get_file(&self, fd: u32) -> Result<Arc<FileEntry>, Error> {
         self.get(fd)
     }
+    fn get_file_mut(&mut self, fd: u32) -> Result<&mut FileEntry, Error> {
+        self.get_mut(fd)
+    }
 }

 pub(crate) struct FileEntry {
@@ -272,6 +276,7 @@ impl FileEntry {

 pub trait FileEntryExt {
     fn get_cap(&self, caps: FileCaps) -> Result<&dyn WasiFile, Error>;
+    fn get_cap_mut(&mut self, caps: FileCaps) -> Result<&mut dyn WasiFile, Error>;
 }

 impl FileEntryExt for FileEntry {
@@ -279,6 +284,10 @@ impl FileEntryExt for FileEntry {
         self.capable_of(caps)?;
         Ok(&*self.file)
     }
+    fn get_cap_mut(&mut self, caps: FileCaps) -> Result<&mut dyn WasiFile, Error> {
+        self.capable_of(caps)?;
+        Ok(&mut *self.file)
+    }
 }

 bitflags! {
diff --git a/crates/wasi-common/src/snapshots/preview_1.rs b/crates/wasi-common/src/snapshots/preview_1.rs
index 4776cb10c..ab2af7d14 100644
--- a/crates/wasi-common/src/snapshots/preview_1.rs
+++ b/crates/wasi-common/src/snapshots/preview_1.rs
@@ -185,9 +185,9 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
         fd: types::Fd,
         flags: types::Fdflags,
     ) -> Result<(), Error> {
-        self.table()
-            .get_file(u32::from(fd))?
-            .get_cap(FileCaps::FDSTAT_SET_FLAGS)?
+        self.table
+            .get_file_mut(u32::from(fd))?
+            .get_cap_mut(FileCaps::FDSTAT_SET_FLAGS)?
             .set_fdflags(FdFlags::from(flags))
             .await
     }
diff --git a/crates/wasi-common/src/table.rs b/crates/wasi-common/src/table.rs
index a92fefff6..7f7939a2f 100644
--- a/crates/wasi-common/src/table.rs
+++ b/crates/wasi-common/src/table.rs
@@ -76,6 +76,21 @@ impl Table {
         }
     }

+    /// TODO
+    pub fn get_mut<T: Any>(&mut self, key: u32) -> Result<&mut T, Error> {
+        let entry = match self.0.get_mut().unwrap().map.get_mut(&key) {
+            Some(entry) => entry,
+            None => return Err(Error::badf().context("key not in table")),
+        };
+        let entry = match Arc::get_mut(entry) {
+            Some(entry) => entry,
+            None => return Err(Error::badf().context("cannot mutably borrow shared file")),
+        };
+        entry
+            .downcast_mut::<T>()
+            .ok_or_else(|| Error::badf().context("element is a different type"))
+    }
+
     /// Remove a resource at a given index from the table. Returns the resource
     /// if it was present.
     pub fn delete<T: Any + Send + Sync>(&self, key: u32) -> Option<Arc<T>> {

</details>

Wasmtime GitHub notifications bot (Feb 07 2023 at 22:40):

alexcrichton closed without merge PR #5326.

Last updated: Nov 22 2024 at 16:03 UTC