Investigate fd-lock failure for spidermonkey-wasm

[yoshuawuyts]

bytecodealliance/spidermonkey-wasm-rs#40 (comment) reported a failure of the fd-lock crate, requiring a dependency to be pinned to an earlier version.

Quote:

From my digging it looks like https://github.com/yoshuawuyts/fd-lock relies on some features that were removed in one of the more recent Rust releases.

[nathaniel-daniel]

Giving the logs a quick look, I see this:

 error[E0255]: the name `unsupported` is defined multiple times
  --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/fd-lock-3.0.8/src/sys/mod.rs:15:17
   |
14 |         mod unsupported;
   |         ---------------- previous definition of the module `unsupported` here
15 |         pub use unsupported;
   |                 ^^^^^^^^^^^ `unsupported` reimported here
   |
   = note: `unsupported` must be defined only once in the type namespace of this module
help: you can use `as` to change the binding name of the import
   |
15 |         pub use unsupported as other_unsupported;
   |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Does the unsupported backend even compile? Also, looks like they are getting errors since they are compiling for wasi, which to my knowledge doesn't have advisory locks? Also, the unsupported backend seems to assume that it's on a unix platform and uses AsRawFd.

Additionally, that crate did not even have fd-lock as a dependency until they updated (or rather auto-updated), their promptly dependency from 0.3.0 to 0.3.1, which updated their rustylinedependency from 6.3.0 all the way to 9.1.2. The version 9.1.2 pulls in fd-lock, while 6.3.0 does not.

I think the only problem with fd-lock is with the unsupported backend not even compiling. The issue with wasi not supporting file locks is something rustyline should solve imo.

[nathaniel-daniel]

I wrote a possible fix for this issue, but I had a hard time rationalizing the panic-based solution instead of a compile-time error.

Basically, locking can be applied to any type that can provide a file handle. However, on platforms that are unsupported, we cannot know what is means to provide a file handle, yet the file type still exists. As a result, platforms that cannot do locking still provide a file type, yet the AsRaw trait is not implemented for them, breaking compilation anyways as locks cannot wrap a type that does not implement AsRaw.

Two possible solutions to this that I can see would be to remove the AsRaw bound for locks on unsupported platforms, but that would mean platform specific details would spread around the crate, wherever AsRaw is used. Code written for unsupported platforms could ironically fail to compile for supported platforms as T is not constrained by AsRaw. Alternatively, we could manually implement AsRaw for File, which think is the best solution, but that might break compilation for some user-defined File wrappers.

It seems impossible to ensure that we can always compile on unsupported platforms so long as locking is based on the AsRaw trait, which is a re-export of the platform-specific method to get a file handle from a type, instead of defining a separate trait in this crate. I was wondering if it would be simpler to just create a compile-time error, or if making a "best-effort" to compile on unsupported platforms is preferred. Slightly related to #18.

If making a best-effort to compile is preferred, would it be fine to convert the panics to errors in the unsupported backend? That seems to be what the standard library does for File.

[yoshuawuyts]

If making a best-effort to compile is preferred, would it be fine to convert the panics to errors in the unsupported backend? That seems to be what the standard library does for File.

Yeah, I'm inclined to say we should follow the lead of the stdlib - even if it's not ideal. Ideally file locking would be exposed by WASI itself, but I suspect that may not happen anytime soon as there are other more important primitives missing still. I'm not really sure tbh; do you have a preference?

[nathaniel-daniel]

Yeah, I'm inclined to say we should follow the lead of the stdlib - even if it's not ideal. Ideally file locking would be exposed by WASI itself, but I suspect that may not happen anytime soon as there are other more important primitives missing still. I'm not really sure tbh; do you have a preference?

I would prefer returning an error instead of panicking if following the stdlib closely is a necessity. If it is not, I would prefer to get a compile error since in a lot of cases, there isn't a graceful way to handle an absence of locks. I was planning on introducing another PR to add this as a feature, like described in #18. I'm currently trying to approach this by treating unsupported platforms as a form of black box; while wasi file locks are not implemented currently (and they could get implemented in the future), some platforms will likely never get file locks like wasm32-unknown-unkown.

However, I think the main issue here is that it is impossible to mimic std in this case. Ideally, we would define our own AsRaw trait, so that it is always present, even on unsupported platforms. Users could then implement this trait for custom types to allow custom types to do locking as well. However, I think making this change would be breaking. In the branch I created earlier, I created a fake AsRaw trait for unsupported platforms. However, this would break compilation, as File does not implement it and user-defined types do not implement it. While manually implementing AsRaw for File is an option, this will still break compilation for user defined types.

I think for the current major version, compiling on an unsupported platform should be a compile error so that users can at least get a nice message that they are compiling on an unsupported platform. This is non-breaking as this crate will not compile on unsupported platforms currently. In the next major version, if compiling on unsupported platforms is necessary, a crate-defined AsRaw trait could be defined so that compilation will always succeed, even on unsupported platforms, instead providing a runtime error.

Basically, I was wondering if it would be fine to convert this crate to emit user-friendly compile errors on unsupported platforms until the next breaking version, where the API could be updated so that it is possible to always compile on unsupported platforms? Or if getting at least locks for the File type to compile on unsupported platforms is preferred?

[yoshuawuyts]

@sunfishcode pinged me yesterday, and told me it's likely WASI will soon have support for file locking APIs. It will likely be implemented using the fd-lock crate, meaning perhaps we can actually wait for that to land and then make use of that? I assumed that wouldn't happen anytime soon, but imo it represents the best-case scenario?

---

I would prefer to get a compile error since in a lot of cases, there isn't a graceful way to handle an absence of locks

In general I'm also in favor of generating a compile-time error. The only reason not to would have been specifically for WASM, where std compiles but returns a run-time error. Given we should be able to address that now through the WASI file locking interface, I think for the remainder of platforms we should just continue to return compile errors? If you know of a better way to create errors for those platforms, def go for it!