Added async port of rust-vfs

[Fredrik-Reinholdsen]

Summary

Implemented and added a one-to-one asynchronous port of the core rust-vfs library.

The async port is added in a new sub-module called async_vfs, and is hidden behind the async-vfs feature flag.

This flag is disabled by default and makes no changes to the default rust-vfs crate. Increasing the minimum supported Rust version is needed however, increased from 1.40.0 to 1.56.0.

All tests and doc-tests for the regular rust-vfs crate have also been ported to async, and all pass.

[manuel-woelker]

Hi there, thanks for the PR! I'll try to have a look at it this weekend.

[manuel-woelker]

It's gonna take me a bit to go through all the changes, and I have to admit I'll have to brush up on my async as well.

The CI currently fails, due to the use of the async_closure feature in stable. Would it be possible to replace them with async blocks?

[Fredrik-Reinholdsen]

Very sorry about the late reply. I was sick this week so did not have the energy to do anything.

Should definitely be able to get rid of the the async closures.

I will try it out and try running the CI locally and push a new commit when that is working.

[manuel-woelker]

No worries, hope you are feeling better!

[Fredrik-Reinholdsen]

Alright! Got rid off all the async closures. Also updated the MSRV in the compile.yaml file in the CI pipeline, which I missed last time around.

I also had to add to clippy directives, to ignore "redundant_async_blocks" warnings. The blocks referenced in the warnings are not actually redundant since they are called and pinned in a sync context to get a poll-able future. Not having those async blocks causes life-time issues.

I also added a clippy directive to ignore warnings about "complex types". The type that triggers this warning is used internally by the async WalkDirIterator to hold spawned futures between calls to poll_next to not spawn a new future on each poll. I figured that ignoring this warning should be fine since it is one place and only used internally, and is not exposed to users of the library.

Let me know if there is anything else you would like me to fix. Again, sorry about the massive PR

[manuel-woelker]

Nice, thanks for the hard work!

It'll probably take me a bit to look through it all - I'll let you know as soon as I have anything.

Edit: Compile looks good, but it seems there are still some minor issues clippy is unhappy about...

[Fredrik-Reinholdsen]

Hi, Again, sorry about the delay, but the CI is now passing. Fixed the remaining formatting issues and changed some versions around.

I downgraded the tokio version from 1.32.0 to 1.29.0 to get the MSRV as low as possible. Unfortunately I had to bump the MSRV up to 1.61.0. The reason for this was some code in the async-recursion dependency that uses some new format strings that came in Rust 1.58, even in the oldest version of the crate.

I had to bump the MSRV further up to 1.61.0 due to some breaking changes in the async-std crate that I was not able to reconcile using Rust 1.58.

I realize that this is a rather large jump in MSRV of the crate, and I am sorry about that.

I guess it might be OK since you only need it with the async-vfs feature enabled?

Thanks for your help and feedback.

[Fredrik-Reinholdsen]

Hi, those are valid points, and I agree with you. I will make these changes and push it. The negative I see with combining some of the sync and async parts, like VfsError is that it means one has to take on some more dependencies, like async_std even if one does not want to use the async parts of the crate.

Although as you say that may be a trade-off worth making for maintainability. I will implement the requested changes and push them some time during the day today. Again sorry about the delay. My day-job has been eating up a lot of time lately.

[Fredrik-Reinholdsen]

Ok,

I have implemented the suggested name-changes of all async file-system implementations and all async versions of the sync vfs types, so that they are now called AsyncPhysicalFS, AsyncMemoryFS etc. On a similar note I also changed the name of some of the types used internally in the async implementations like MemoryFile to AsyncMemory file etc.

I have also gotten rid of some redundant types in the async module. The VfsResult, VfsError, `VfsMetadata and VfsFileType types to be specific. The async-vfs module now imports these from the top level crate instead.

I have not defined a new common trait to try and consolidate some of the very similar functions from VfsPath and AsyncVfsPath. I am open to doing so, but it should be considered that this will break backwards compatibility since one would have to also import this trait to use the trait methods. I did not want to introduce any breaking changes without explicit permission from you.

Do you think it is worth doing anyways?

Thanks!

[manuel-woelker]

Thanks for implementing the changes :+1: Looks very good!

Regarding consolidating the common functionality: a good way might be to simply factor out the common logic internally, while leaving the public API unchanged & independent. So for example introducing a private utility function for path joining, that is internally used by both sync and async implementations. But that should be an implementation detail and not publicly visible.

[Fredrik-Reinholdsen]

Alright, Added a new internal common trait called PathLike, which consolidates some of the common sync functionality of AsyncVfs and VfsPath.

The amount of code that can be consolidated is not super high, due to async/sync not mixing well without considerable refactoring of the whole crate. This common trait does improve things a bit though, most notably by containing the join_internal, and some other functions like parent etc.

Let me know what you think. Thanks!

[manuel-woelker]

Thanks for the update! I will take a closer look this weekend!

[manuel-woelker]

Merged and published. Again a huge thank you for the work and implementing the changes! 👍

[Fredrik-Reinholdsen]

Thanks, and thank you for a great crate!

[abhemanyus]

Hi, Again, sorry about the delay, but the CI is now passing. Fixed the remaining formatting issues and changed some versions around.

I downgraded the tokio version from 1.32.0 to 1.29.0 to get the MSRV as low as possible. Unfortunately I had to bump the MSRV up to 1.61.0. The reason for this was some code in the async-recursion dependency that uses some new format strings that came in Rust 1.58, even in the oldest version of the crate.

I had to bump the MSRV further up to 1.61.0 due to some breaking changes in the async-std crate that I was not able to reconcile using Rust 1.58.

I realize that this is a rather large jump in MSRV of the crate, and I am sorry about that.

I guess it might be OK since you only need it with the async-vfs feature enabled?

Thanks for your help and feedback.

Will locking tokio to 1.29.0 not conflict with projects using a more up-to-date version?

[manuel-woelker]

Hmm, that might be an issue. I think I remember some cargo mechanism to override the used version, which might address that issue, at least until breaking version changes.

To be honest, I am not all that knowledgeable about this particular case of dependency hell.

How do other projects handle this? Is there a recommended practice?

[abhemanyus]

Hmm, that might be an issue. I think I remember some cargo mechanism to override the used version, which might address that issue, at least until breaking version changes.

To be honest, I am not all that knowledgeable about this particular case of dependency hell.

How do other projects handle this? Is there a recommended practice?

you could leave it up to cargo by not strictly setting the required tokio version. It works just fine on my fork https://github.com/abhemanyus/rust-vfs/blob/master/Cargo.toml

[morphmeme]

Are there any plans to unlocking the version of tokio to not 1.29.0? I'm currently blocked since our project requires a newer version of tokio.

[Fredrik-Reinholdsen]

Hi, I would certainly be willing to have a look at this.

If i remember correctly, the reason for selecting tokio 1.29 was originally me trying to minimize the increase in the minimum supported Rust version (MSRV). Some major changes in tokio 1.3 made some of the async dependencies incompatible without increasing the MSRV by quite a bit. But other than that there should not be any issues with increasing the tokio version.

I can make a PR with bumped up the versions of the async dependencies and we can review it.

What do you think @manuel-woelker ?

[manuel-woelker]

Are there any drawbacks to unlocking the tokio version (i.e. going from "=1.29.0" to `"1.29.0")?

That would allow downstream consumers to use newer tokio versions while still supporting older Rust versions.

If there are no disadvantages, this is what I would prefer for now. If there are issues with that however, I think bumping the MSRV might be sensible as well.