Skip to content

Commit

Permalink
[WasmFS] Allow backends to report errors from flush
Browse files Browse the repository at this point in the history
And catch and report such errors in the OPFS backend to prevent errors from
causing the async task to hang and create deadlocks.
  • Loading branch information
tlively committed Sep 1, 2022
1 parent f449ced commit 5dffb10
Show file tree
Hide file tree
Showing 12 changed files with 74 additions and 64 deletions.
10 changes: 7 additions & 3 deletions src/library_wasmfs_opfs.js
Original file line number Diff line number Diff line change
Expand Up @@ -402,10 +402,14 @@ mergeInto(LibraryManager.library, {
},

_wasmfs_opfs_flush_access__deps: ['$wasmfsOPFSAccessHandles'],
_wasmfs_opfs_flush_access: async function(ctx, accessID) {
_wasmfs_opfs_flush_access: async function(ctx, accessID, errPtr) {
let accessHandle = wasmfsOPFSAccessHandles.get(accessID);
// TODO: Error handling
await accessHandle.flush();
try {
await accessHandle.flush();
} catch {
let err = -{{{ cDefine('EIO') }}};
{{{ makeSetValue('errPtr', 0, 'err', 'i32') }}};
}
_emscripten_proxy_finish(ctx);
}
});
8 changes: 4 additions & 4 deletions system/lib/wasmfs/backends/node_backend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ class NodeFile : public DataFile {
: DataFile(mode, backend), state(path) {}

private:
size_t getSize() override {
off_t getSize() override {
// TODO: This should really be using a 64-bit file size type.
uint32_t size;
if (state.isOpen()) {
Expand All @@ -151,10 +151,10 @@ class NodeFile : public DataFile {
return 0;
}
}
return size_t(size);
return off_t(size);
}

int setSize(size_t size) override {
int setSize(off_t size) override {
WASMFS_UNREACHABLE("TODO: implement NodeFile::setSize");
}

Expand All @@ -179,7 +179,7 @@ class NodeFile : public DataFile {
return nwritten;
}

void flush() override {
int flush() override {
WASMFS_UNREACHABLE("TODO: implement NodeFile::flush");
}
};
Expand Down
30 changes: 16 additions & 14 deletions system/lib/wasmfs/backends/opfs_backend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -88,26 +88,25 @@ int _wasmfs_opfs_write_access(int access_id,
// Get the size via an AccessHandle.
void _wasmfs_opfs_get_size_access(em_proxying_ctx* ctx,
int access_id,
uint32_t* size);
off_t* size);

// TODO: return 64-byte off_t.
uint32_t _wasmfs_opfs_get_size_blob(int blob_id);

// Get the size of a file handle via a File Blob.
void _wasmfs_opfs_get_size_file(em_proxying_ctx* ctx,
int file_id,
uint32_t* size);
void _wasmfs_opfs_get_size_file(em_proxying_ctx* ctx, int file_id, off_t* size);

void _wasmfs_opfs_set_size_access(em_proxying_ctx* ctx,
int access_id,
uint32_t size,
off_t size,
int* err);

void _wasmfs_opfs_set_size_file(em_proxying_ctx* ctx,
int file_id,
uint32_t size,
off_t size,
int* err);

void _wasmfs_opfs_flush_access(em_proxying_ctx* ctx, int access_id);
void _wasmfs_opfs_flush_access(em_proxying_ctx* ctx, int access_id, int* err);

} // extern "C"

Expand Down Expand Up @@ -222,9 +221,8 @@ class OPFSFile : public DataFile {
}

private:
size_t getSize() override {
// TODO: 64-bit sizes.
uint32_t size;
off_t getSize() override {
off_t size;
switch (state.getKind()) {
case OpenState::None:
proxy([&](auto ctx) {
Expand All @@ -242,10 +240,10 @@ class OPFSFile : public DataFile {
default:
WASMFS_UNREACHABLE("Unexpected open state");
}
return size_t(size);
return size;
}

int setSize(size_t size) override {
int setSize(off_t size) override {
int err = 0;
switch (state.getKind()) {
case OpenState::Access:
Expand All @@ -259,6 +257,8 @@ class OPFSFile : public DataFile {
// become invalidated and refreshing it while ensuring other in-flight
// operations on the same file do not observe the invalidated blob would
// be extremely complicated.
// TODO: Can we assume there are no other in-flight operations on this
// file and do something better here?
return -EIO;
case OpenState::None: {
proxy([&](auto ctx) {
Expand Down Expand Up @@ -313,18 +313,20 @@ class OPFSFile : public DataFile {
return nwritten;
}

void flush() override {
int flush() override {
int err = 0;
switch (state.getKind()) {
case OpenState::Access:
proxy([&](auto ctx) {
_wasmfs_opfs_flush_access(ctx.ctx, state.getAccessID());
_wasmfs_opfs_flush_access(ctx.ctx, state.getAccessID(), &err);
});
break;
case OpenState::Blob:
case OpenState::None:
default:
break;
}
return err;
}
};

Expand Down
8 changes: 4 additions & 4 deletions system/lib/wasmfs/backends/proxied_file_backend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,17 @@ class ProxiedFile : public DataFile {
return result;
}

void flush() override {}
int flush() override { return 0; }

// Querying the size of the Proxied File returns the size of the underlying
// file given by the proxying mechanism.
size_t getSize() override {
size_t result;
off_t getSize() override {
off_t result;
proxy([&]() { result = baseFile->locked().getSize(); });
return result;
}

int setSize(size_t size) override {
int setSize(off_t size) override {
WASMFS_UNREACHABLE("TODO: ProxiedFS setSize");
}

Expand Down
22 changes: 12 additions & 10 deletions system/lib/wasmfs/file.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,9 @@ class File : public std::enable_shared_from_this<File> {
// A mutex is needed for multiple accesses to the same file.
std::recursive_mutex mutex;

// May be called on files that have not been opened.
virtual size_t getSize() = 0;
// The the size in bytes of a file or return a negative error code. May be
// called on files that have not been opened.
virtual off_t getSize() = 0;

mode_t mode = 0; // User and group mode bits for access permission.

Expand Down Expand Up @@ -144,10 +145,11 @@ class DataFile : public File {
// Sets the size of the file to a specific size. If new space is allocated, it
// should be zero-initialized. May be called on files that have not been
// opened. Returns 0 on success or a negative error code.
virtual int setSize(size_t size) = 0;
virtual int setSize(off_t size) = 0;

// TODO: Design a proper API for flushing files.
virtual void flush() = 0;
// Sync the file data to the underlying persistent storage, if any. Returns 0
// on success or a negative error code.
virtual int flush() = 0;

public:
static constexpr FileKind expectedKind = File::DataFileKind;
Expand Down Expand Up @@ -253,7 +255,7 @@ class Directory : public File {
protected:
// 4096 bytes is the size of a block in ext4.
// This value was also copied from the JS file system.
size_t getSize() override { return 4096; }
off_t getSize() override { return 4096; }
};

class Symlink : public File {
Expand All @@ -269,7 +271,7 @@ class Symlink : public File {
virtual std::string getTarget() const = 0;

protected:
size_t getSize() override { return getTarget().size(); }
off_t getSize() override { return getTarget().size(); }
};

class File::Handle {
Expand All @@ -285,7 +287,7 @@ class File::Handle {
Handle(std::shared_ptr<File> file) : file(file), lock(file->mutex) {}
Handle(std::shared_ptr<File> file, std::defer_lock_t)
: file(file), lock(file->mutex, std::defer_lock) {}
size_t getSize() { return file->getSize(); }
off_t getSize() { return file->getSize(); }
mode_t getMode() { return file->mode; }
void setMode(mode_t mode) {
// The type bits can never be changed (whether something is a file or a
Expand Down Expand Up @@ -324,10 +326,10 @@ class DataFile::Handle : public File::Handle {
return getFile()->write(buf, len, offset);
}

[[nodiscard]] int setSize(size_t size) { return getFile()->setSize(size); }
[[nodiscard]] int setSize(off_t size) { return getFile()->setSize(size); }

// TODO: Design a proper API for flushing files.
void flush() { getFile()->flush(); }
[[nodiscard]] int flush() { return getFile()->flush(); }

// This function loads preloaded files from JS Memory into this DataFile.
// TODO: Make this virtual so specific backends can specialize it for better
Expand Down
6 changes: 3 additions & 3 deletions system/lib/wasmfs/js_impl_backend.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,13 @@ class JSImplFile : public DataFile {
getBackendIndex(), getFileIndex(), buf, len, offset);
}

void flush() override {}
int flush() override { return 0; }

size_t getSize() override {
off_t getSize() override {
return _wasmfs_jsimpl_get_size(getBackendIndex(), getFileIndex());
}

int setSize(size_t size) override {
int setSize(off_t size) override {
WASMFS_UNREACHABLE("TODO: JSImpl setSize");
}

Expand Down
6 changes: 3 additions & 3 deletions system/lib/wasmfs/memory_backend.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ class MemoryFile : public DataFile {
int close() override { return 0; }
ssize_t write(const uint8_t* buf, size_t len, off_t offset) override;
ssize_t read(uint8_t* buf, size_t len, off_t offset) override;
void flush() override {}
size_t getSize() override { return buffer.size(); }
int setSize(size_t size) override {
int flush() override { return 0; }
off_t getSize() override { return buffer.size(); }
int setSize(off_t size) override {
buffer.resize(size);
return 0;
}
Expand Down
6 changes: 3 additions & 3 deletions system/lib/wasmfs/pipe_backend.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,12 @@ class PipeFile : public DataFile {
return len;
}

void flush() override {}
int flush() override { return 0; }

size_t getSize() override { return data->size(); }
off_t getSize() override { return data->size(); }

// TODO: Should this return an error?
int setSize(size_t size) override { return 0; }
int setSize(off_t size) override { return 0; }

public:
// PipeFiles do not have or need a backend. Pass NullBackend to the parent for
Expand Down
10 changes: 5 additions & 5 deletions system/lib/wasmfs/proxied_async_js_impl_backend.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ void _wasmfs_jsimpl_async_read(em_proxying_ctx* ctx,
void _wasmfs_jsimpl_async_get_size(em_proxying_ctx* ctx,
js_index_t backend,
js_index_t index,
size_t* result);
off_t* result);
}

namespace wasmfs {
Expand Down Expand Up @@ -106,18 +106,18 @@ class ProxiedAsyncJSImplFile : public DataFile {
return result;
}

void flush() override {}
int flush() override { return 0; }

size_t getSize() override {
size_t result;
off_t getSize() override {
off_t result;
proxy([&](auto ctx) {
_wasmfs_jsimpl_async_get_size(
ctx.ctx, getBackendIndex(), getFileIndex(), &result);
});
return result;
}

int setSize(size_t size) override {
int setSize(off_t size) override {
WASMFS_UNREACHABLE("TODO: ProxiedAsyncJSImplFile setSize");
}

Expand Down
25 changes: 13 additions & 12 deletions system/lib/wasmfs/special_files.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ class NullFile : public DataFile {

ssize_t read(uint8_t* buf, size_t len, off_t offset) override { return 0; }

void flush() override {}
size_t getSize() override { return 0; }
int setSize(size_t size) override { return -EPERM; }
int flush() override { return 0; }
off_t getSize() override { return 0; }
int setSize(off_t size) override { return -EPERM; }

public:
NullFile() : DataFile(S_IRUGO | S_IWUGO, NullBackend, S_IFCHR) {}
Expand All @@ -49,9 +49,9 @@ class StdinFile : public DataFile {
abort();
};

void flush() override {}
size_t getSize() override { return 0; }
int setSize(size_t size) override { return -EPERM; }
int flush() override { return 0; }
off_t getSize() override { return 0; }
int setSize(off_t size) override { return -EPERM; }

public:
StdinFile() : DataFile(S_IRUGO, NullBackend, S_IFCHR) { seekable = false; }
Expand All @@ -69,16 +69,17 @@ class WritingStdFile : public DataFile {
return -__WASI_ERRNO_INVAL;
};

void flush() override {
int flush() override {
// Write a null to flush the output if we have content.
if (!writeBuffer.empty()) {
const uint8_t nothing = '\0';
write(&nothing, 1, 0);
}
return 0;
}

size_t getSize() override { return 0; }
int setSize(size_t size) override { return -EPERM; }
off_t getSize() override { return 0; }
int setSize(off_t size) override { return -EPERM; }

ssize_t writeToJS(const uint8_t* buf,
size_t len,
Expand Down Expand Up @@ -150,9 +151,9 @@ class RandomFile : public DataFile {
return len;
};

void flush() override {}
size_t getSize() override { return 0; }
int setSize(size_t size) override { return -EPERM; }
int flush() override { return 0; }
off_t getSize() override { return 0; }
int setSize(off_t size) override { return -EPERM; }

public:
RandomFile() : DataFile(S_IRUGO, NullBackend, S_IFCHR) { seekable = false; }
Expand Down
3 changes: 2 additions & 1 deletion system/lib/wasmfs/syscalls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,8 @@ __wasi_errno_t __wasi_fd_sync(__wasi_fd_t fd) {
// way. TODO: in the future we may want syncing of directories.
auto dataFile = openFile->locked().getFile()->dynCast<DataFile>();
if (dataFile) {
dataFile->locked().flush();
// Translate to WASI standard of positive return codes.
return -dataFile->locked().flush();
}

return __WASI_ERRNO_SUCCESS;
Expand Down
4 changes: 2 additions & 2 deletions system/lib/wasmfs/wasmfs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ WasmFS::~WasmFS() {
// Note that we lock here, although strictly speaking it is unnecessary given
// that we are in the destructor of WasmFS: nothing can possibly be running
// on files at this time.
SpecialFiles::getStdout()->locked().flush();
SpecialFiles::getStderr()->locked().flush();
(void)SpecialFiles::getStdout()->locked().flush();
(void)SpecialFiles::getStderr()->locked().flush();

// Break the reference cycle caused by the root directory being its own
// parent.
Expand Down

0 comments on commit 5dffb10

Please sign in to comment.