feat: remove mutability from the WasiCtx Table

[haraldh]

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: #5235

[github-actions]

Subscribe to Label Action

cc @kubkon

This issue or pull request has been labeled: "wasi"

Thus the following users have been cc'd because of the following labels:

• kubkon: wasi

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[bjorn3]

This will result in a performance regression due to all these locking operations, right? Are you making this change to add multi-threading support?

[alexcrichton]

Have you been able to run any benchmarks to see the effect of this change?

[alexcrichton]

How come these need changing? The prior signatures I think should work?

[haraldh]

In the method a RwLockWriteGuard<BorrowedFd> would be produced, which can't be dereferenced for return.

[alexcrichton]

Sorry let me restate. From a system API perspective a lock is not needed here, so I don't know where the rwlock is motivated from. I would expect that these signatures remain the same since this is intended for bottoming out in the underlying platform-specific interface which handles all synchronization internally in the kernel.

[haraldh]

It's only because of File::set_fd_flags() needing &mut self

[alexcrichton]

I think ideally this is avoided because a system file doesn't need an extra mutex around it since it should already be able to perform all necessary operations concurrently.

[haraldh]

Well, it's only cap_std::fs::File::set_fd_flags(&mut self, set_fd_flags: SetFdFlags<Self>) -> io::Result<()> which needs it.

[haraldh]

Maybe, we should start removing the mut there.

[haraldh]

seems to be in crate system-interface ... trait GetSetFdFlags

[haraldh]

because of windows... woah... wildcard impl for all T restricted per method.

#[cfg(windows)]
impl<T> GetSetFdFlags for T {
    /// …
    fn set_fd_flags(&mut self, set_fd_flags: SetFdFlags<Self>) -> io::Result<()>
    where
        Self: AsFilelike,
    {
        *self = set_fd_flags.reopened;
        Ok(())
    }

[haraldh]

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.

[alexcrichton]

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.

[bstrie]

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.

[abrown]

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): abrown/wasmtime@pr-5326...abrown:wasmtime:pr-5326-arc. @alexcrichton can you take a look?

[alexcrichton]

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:

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>> {

[abrown]

@haraldh, you should now be able to run the applicable benchmarks with something like cargo bench wasi/ from the top-level project directory.

[haraldh]

Have you been able to run any benchmarks to see the effect of this change?

I haven't seen any significant change (-2% at most), but the wasi benches have a relatively big difference from run to run anyway.

[alexcrichton]

This has now all landed in-spirit at least with #5484 so I'm going to close this. Thanks for the work here though @haraldh!