Skip to content

feat: remove mutability from the WasiCtx Table #5326

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

feat: remove mutability from the WasiCtx Table #5326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 89 additions & 71 deletions crates/wasi-common/cap-std-sync/src/file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use is_terminal::IsTerminal;
use std::any::Any;
use std::convert::TryInto;
use std::io;
use std::sync::{Arc, RwLock, RwLockReadGuard};
use system_interface::{
fs::{FileIoExt, GetSetFdFlags},
io::{IoExt, ReadReady},
Expand All @@ -14,11 +15,48 @@ use wasi_common::{
Error, ErrorExt,
};

pub struct File(cap_std::fs::File);
#[cfg(unix)]
use io_lifetimes::{AsFd, BorrowedFd};

#[cfg(windows)]
use io_lifetimes::{AsHandle, BorrowedHandle};

#[cfg(windows)]
use io_extras::os::windows::{AsRawHandleOrSocket, RawHandleOrSocket};

pub struct BorrowedFile<'a>(RwLockReadGuard<'a, cap_std::fs::File>);

#[cfg(unix)]
impl AsFd for BorrowedFile<'_> {
fn as_fd(&self) -> BorrowedFd<'_> {
self.0.as_fd()
}
}

#[cfg(windows)]
impl AsHandle for BorrowedFile<'_> {
fn as_handle(&self) -> BorrowedHandle<'_> {
self.0.as_handle()
}
}

#[cfg(windows)]
impl AsRawHandleOrSocket for BorrowedFile<'_> {
#[inline]
fn as_raw_handle_or_socket(&self) -> RawHandleOrSocket {
self.0.as_raw_handle_or_socket()
}
}

pub struct File(RwLock<cap_std::fs::File>);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@haraldh haraldh Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, we should start removing the mut there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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(())
    }

Copy link
Contributor Author

@haraldh haraldh Dec 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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


impl File {
pub fn from_cap_std(file: cap_std::fs::File) -> Self {
File(file)
File(RwLock::new(file))
}

pub fn borrow(&self) -> BorrowedFile {
BorrowedFile(self.0.read().unwrap())
}
}

Expand All @@ -27,45 +65,49 @@ impl WasiFile for File {
fn as_any(&self) -> &dyn Any {
self
}

#[cfg(unix)]
fn pollable(&self) -> Option<rustix::fd::BorrowedFd> {
Some(self.0.as_fd())
fn pollable(&self) -> Option<Arc<dyn AsFd + '_>> {
Some(Arc::new(self.borrow()))
}

#[cfg(windows)]
fn pollable(&self) -> Option<io_extras::os::windows::RawHandleOrSocket> {
Some(self.0.as_raw_handle_or_socket())
fn pollable(&self) -> Option<Arc<dyn AsRawHandleOrSocket + '_>> {
Some(Arc::new(BorrowedFile(self.0.read().unwrap())))
}
async fn datasync(&mut self) -> Result<(), Error> {
self.0.sync_data()?;

async fn datasync(&self) -> Result<(), Error> {
self.0.read().unwrap().sync_data()?;
Ok(())
}
async fn sync(&mut self) -> Result<(), Error> {
self.0.sync_all()?;
async fn sync(&self) -> Result<(), Error> {
self.0.read().unwrap().sync_all()?;
Ok(())
}
async fn get_filetype(&mut self) -> Result<FileType, Error> {
let meta = self.0.metadata()?;
async fn get_filetype(&self) -> Result<FileType, Error> {
let meta = self.0.read().unwrap().metadata()?;
Ok(filetype_from(&meta.file_type()))
}
async fn get_fdflags(&mut self) -> Result<FdFlags, Error> {
let fdflags = get_fd_flags(&self.0)?;
async fn get_fdflags(&self) -> Result<FdFlags, Error> {
let file = self.0.read().unwrap();
let fdflags = get_fd_flags(&*file)?;
Ok(fdflags)
}
async fn set_fdflags(&mut self, fdflags: FdFlags) -> Result<(), Error> {
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 set_fd_flags = self.0.new_set_fd_flags(to_sysif_fdflags(fdflags))?;
self.0.set_fd_flags(set_fd_flags)?;
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(())
}
async fn get_filestat(&mut self) -> Result<Filestat, Error> {
let meta = self.0.metadata()?;
async fn get_filestat(&self) -> Result<Filestat, Error> {
let meta = self.0.read().unwrap().metadata()?;
Ok(Filestat {
device_id: meta.dev(),
inode: meta.ino(),
Expand All @@ -77,63 +119,68 @@ impl WasiFile for File {
ctim: meta.created().map(|t| Some(t.into_std())).unwrap_or(None),
})
}
async fn set_filestat_size(&mut self, size: u64) -> Result<(), Error> {
self.0.set_len(size)?;
async fn set_filestat_size(&self, size: u64) -> Result<(), Error> {
self.0.read().unwrap().set_len(size)?;
Ok(())
}
async fn advise(&mut self, offset: u64, len: u64, advice: Advice) -> Result<(), Error> {
self.0.advise(offset, len, convert_advice(advice))?;
async fn advise(&self, offset: u64, len: u64, advice: Advice) -> Result<(), Error> {
self.0
.read()
.unwrap()
.advise(offset, len, convert_advice(advice))?;
Ok(())
}
async fn allocate(&mut self, offset: u64, len: u64) -> Result<(), Error> {
self.0.allocate(offset, len)?;
async fn allocate(&self, offset: u64, len: u64) -> Result<(), Error> {
self.0.read().unwrap().allocate(offset, len)?;
Ok(())
}
async fn set_times(
&mut self,
&self,
atime: Option<wasi_common::SystemTimeSpec>,
mtime: Option<wasi_common::SystemTimeSpec>,
) -> Result<(), Error> {
self.0
.read()
.unwrap()
.set_times(convert_systimespec(atime), convert_systimespec(mtime))?;
Ok(())
}
async fn read_vectored<'a>(&mut self, bufs: &mut [io::IoSliceMut<'a>]) -> Result<u64, Error> {
let n = self.0.read_vectored(bufs)?;
async fn read_vectored<'a>(&self, bufs: &mut [io::IoSliceMut<'a>]) -> Result<u64, Error> {
let n = self.0.read().unwrap().read_vectored(bufs)?;
Ok(n.try_into()?)
}
async fn read_vectored_at<'a>(
&mut self,
&self,
bufs: &mut [io::IoSliceMut<'a>],
offset: u64,
) -> Result<u64, Error> {
let n = self.0.read_vectored_at(bufs, offset)?;
let n = self.0.read().unwrap().read_vectored_at(bufs, offset)?;
Ok(n.try_into()?)
}
async fn write_vectored<'a>(&mut self, bufs: &[io::IoSlice<'a>]) -> Result<u64, Error> {
let n = self.0.write_vectored(bufs)?;
async fn write_vectored<'a>(&self, bufs: &[io::IoSlice<'a>]) -> Result<u64, Error> {
let n = self.0.read().unwrap().write_vectored(bufs)?;
Ok(n.try_into()?)
}
async fn write_vectored_at<'a>(
&mut self,
&self,
bufs: &[io::IoSlice<'a>],
offset: u64,
) -> Result<u64, Error> {
let n = self.0.write_vectored_at(bufs, offset)?;
let n = self.0.read().unwrap().write_vectored_at(bufs, offset)?;
Ok(n.try_into()?)
}
async fn seek(&mut self, pos: std::io::SeekFrom) -> Result<u64, Error> {
Ok(self.0.seek(pos)?)
async fn seek(&self, pos: std::io::SeekFrom) -> Result<u64, Error> {
Ok(self.0.read().unwrap().seek(pos)?)
}
async fn peek(&mut self, buf: &mut [u8]) -> Result<u64, Error> {
let n = self.0.peek(buf)?;
async fn peek(&self, buf: &mut [u8]) -> Result<u64, Error> {
let n = self.0.read().unwrap().peek(buf)?;
Ok(n.try_into()?)
}
async fn num_ready_bytes(&self) -> Result<u64, Error> {
Ok(self.0.num_ready_bytes()?)
fn num_ready_bytes(&self) -> Result<u64, Error> {
Ok(self.0.read().unwrap().num_ready_bytes()?)
}
fn isatty(&mut self) -> bool {
self.0.is_terminal()
fn isatty(&self) -> bool {
self.0.read().unwrap().is_terminal()
}
}

Expand All @@ -160,35 +207,6 @@ pub fn filetype_from(ft: &cap_std::fs::FileType) -> FileType {
}
}

#[cfg(windows)]
use io_lifetimes::{AsHandle, BorrowedHandle};
#[cfg(windows)]
impl AsHandle for File {
fn as_handle(&self) -> BorrowedHandle<'_> {
self.0.as_handle()
}
}

#[cfg(windows)]
use io_extras::os::windows::{AsRawHandleOrSocket, RawHandleOrSocket};
#[cfg(windows)]
impl AsRawHandleOrSocket for File {
#[inline]
fn as_raw_handle_or_socket(&self) -> RawHandleOrSocket {
self.0.as_raw_handle_or_socket()
}
}

#[cfg(unix)]
use io_lifetimes::{AsFd, BorrowedFd};

#[cfg(unix)]
impl AsFd for File {
fn as_fd(&self) -> BorrowedFd<'_> {
self.0.as_fd()
}
}

pub(crate) fn convert_systimespec(
t: Option<wasi_common::SystemTimeSpec>,
) -> Option<SystemTimeSpec> {
Expand Down
10 changes: 5 additions & 5 deletions crates/wasi-common/cap-std-sync/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,15 @@ impl WasiCtxBuilder {
}
Ok(self)
}
pub fn stdin(mut self, f: Box<dyn WasiFile>) -> Self {
pub fn stdin(self, f: Box<dyn WasiFile>) -> Self {
self.0.set_stdin(f);
self
}
pub fn stdout(mut self, f: Box<dyn WasiFile>) -> Self {
pub fn stdout(self, f: Box<dyn WasiFile>) -> Self {
self.0.set_stdout(f);
self
}
pub fn stderr(mut self, f: Box<dyn WasiFile>) -> Self {
pub fn stderr(self, f: Box<dyn WasiFile>) -> Self {
self.0.set_stderr(f);
self
}
Expand All @@ -118,12 +118,12 @@ impl WasiCtxBuilder {
pub fn inherit_stdio(self) -> Self {
self.inherit_stdin().inherit_stdout().inherit_stderr()
}
pub fn preopened_dir(mut self, dir: Dir, guest_path: impl AsRef<Path>) -> Result<Self, Error> {
pub fn preopened_dir(self, dir: Dir, guest_path: impl AsRef<Path>) -> Result<Self, Error> {
let dir = Box::new(crate::dir::Dir::from_cap_std(dir));
self.0.push_preopened_dir(dir, guest_path)?;
Ok(self)
}
pub fn preopened_socket(mut self, fd: u32, socket: impl Into<Socket>) -> Result<Self, Error> {
pub fn preopened_socket(self, fd: u32, socket: impl Into<Socket>) -> Result<Self, Error> {
let socket: Socket = socket.into();
let file: Box<dyn WasiFile> = socket.into();

Expand Down
Loading