add async support using bisync crate

[Be-ing]

This moves the existing API into a blocking module and duplicates the API into a new asynchronous module, using async fn where applicable. This uses the bisync crate rather than maybe_async used by #154. This involved moving everything in the src directory into a new src/inner directory, then exposing blocking/async versions via how they are imported in src/lib.rs.

I think this resolves the concerns brought up in #154: sync and async can both be built at the same time, usage of bisync by other crates in the dependency graph doesn't affect this, and changes to the example and test code are minimal (just adjusting import path).

I moved submodules that have no difference between blocking & async to a new common module. That code gets re-exported under the blocking/asynchronous modules in the same relative path in the module hierarchy as before. There may be opportunities to split more code into that common module, but I have tried to keep changes to a minimum for now to make it easier to review.

I have not added Cargo features for sync/async; they're both built. If requested, I could put each behind a feature gate, though I don't think it's necessary.

Fixes #50

[thejpster]

An interesting approach! I want to dig into the docs in more detail but it seems workable - modulo that AFIT feature-flag that has appeared.

[thejpster]

It might also be good to move the example import renames to another PR to keep this one cleaner so we can see precisely only that which is affected by the async additions.

[Be-ing]

It might also be good to move the example import renames to another PR to keep this one cleaner so we can see precisely only that which is affected by the async additions.

That goes towards a bigger question: do you want to break the public API by moving the existing API into a new blocking module? This can be avoided by making the blocking module private and instead putting pub use blocking::*; in src/lib.rs. I don't think that would be great for downstream users in the long term though. My primary concern is that it would make the documentation confusing to navigate. Here's how the front page of the documentation would look with that:

There's the whole blocking API of the crate, then the asynchronous module... and when you go to the documentation for that, you see the same API:

I think this would be rather confusing. It's difficult to even tell that I clicked a link and navigated to another page because it looks so similar to the documentation's front page.

By moving the existing API into a blocking module (how this branch currently is), the front page of the documentation looks like this:

[Be-ing]

I've avoided running cargo fmt for now to minimize the diff because it's rather big already. If you'd like that to be run now, just let me know, or I could do it later after you've reviewed.

[Be-ing]

I had to use the #[only_sync] macro in a few places. Those are the impl Drop for File and Volume; unfortunately, there is no such thing (yet) in Rust as an async drop, so that's the best we can do. VolumeManager::close_file and VolumeManager::close_file can be invoked manually though. I presume bad things would happen if those aren't closed manually? If so, I can add a note to the documentation that has to be done manually for the async versions.

The other places I used #[only_sync] were just to make the tests simple to implement.

[thejpster]

I presume bad things would happen if those aren't closed manually?

Yes, leaking a file handle means you can't reopen it, and handles are a finite resource. Plus certain context is only written to disk on file close so the disk ends up needing a fsck/scandisk pass.

As long as that only affects the async users that's fine.

Moving the blocking API into the blocking module is fine with me.

[thejpster]

Can you rebase? #177 might make this smaller.

[thejpster]

Is it possible to give the async version a blocking drop? Better than doing nothing.

[Be-ing]

Can you rebase? #177 might make this smaller.

Okay, rebased.

Is it possible to give the async version a blocking drop? Better than doing nothing.

Unfortunately not. That would require this crate to bring in its own async executor and set that up just within impl Drop. It's questionable if that would be desirable if it was even possible. In a library that uses std, you can wrap async functions with a simple async executor like futures_lite::future::block_on or pollster::block_on, but both of those use std modules like std::future and std::sync that aren't available in core.

[thejpster]

Is there no no_std block_on! ? Can't you just spin polling the Future?

[Be-ing]

I just found embassy_futures::block_on, however, I don't think it's a good idea to use it to impl Drop for async Files/Volumes. We can't know if a user would want that and there's no way to opt out of Drop::drop getting called if it's implemented. For example, the user could have strict power consumption requirements that require putting the MCU to sleep on every .await as Embassy and RTIC do.

[thejpster]

But it's probably better than a corrupt file system, and any correct program will close them asynchronously before dropping.

What do other async File APIs do?

[Be-ing]

tokio::fs::File:

A file will not be closed immediately when it goes out of scope if there are any IO operations that have not yet completed. To ensure that a file is closed immediately when it is dropped, you should call flush before dropping it. Note that this does not ensure that the file has been fully written to disk; the operating system might keep the changes around in an in-memory buffer. See the sync_all method for telling the OS to write the data to disk.

async-std::fs::File:

Files are automatically closed when they get dropped and any errors detected on closing are ignored. Use the sync_all method before dropping a file if such errors need to be handled.

impl Drop for File in async-std:

impl Drop for File {
    fn drop(&mut self) {
        // We need to flush the file on drop. Unfortunately, that is not possible to do in a
        // non-blocking fashion, but our only other option here is losing data remaining in the
        // write cache. Good task schedulers should be resilient to occasional blocking hiccups in
        // file destructors so we don't expect this to be a common problem in practice.
        let _ = futures_lite::future::block_on(self.flush());
    }
}

Suggestion: impl Drop for async File/Volume using embassy_futures::block_on, but put that behind an async_drop Cargo feature that is enabled by default. This will Just Work by default, and any project where that little bit of blocking is really a problem has an escape hatch.

[Be-ing]

Suggestion: impl Drop for async File/Volume using embassy_futures::block_on, but put that behind an async_drop Cargo feature that is enabled by default. This will Just Work by default, and any project where that little bit of blocking is really a problem has an escape hatch.

Nevermind, I forgot core::mem::forget exists to opt out of Drop, and of course File/Volume::close already use it. There's no need for a Cargo feature to prevent dropping; that can be opted out of just by calling File/Volume::close.