Support async locking

[bonsairobo]

Right now, there are blocking methods of AsyncFileExt like lock_shared and lock_exclusive that could be async for implementations like async_fs or tokio.

I can't see a simple way to do this as a user without wrapping async_fs::File in yet another Arc to pass it into blocking::unblock.

[al8n]

Hi, thanks for opening an issue.

Could you give a code example? I do not quite get the point.

[bonsairobo]

@al8n Sure!

Because lock_shared and lock_exclusive may cause the OS to sleep the thread, this would negatively impact performance in a thread-per-core async runtime. So the FileExt trait is not well-suited for async usage, which seems to conflict with the fact that it is implemented for async_fs::File.

In order to avoid this, I would use the blocking crate, which provides a thread pool specifically for blocking IO like this.

let f = Arc::new(async_fs::File::open("lockfile"));
let f2 = f.clone();
unblock(move || f2.lock_shared()).await?;

// Now `f` refers to a locked file handle.

As you can see, unblock requires a 'static closure, so we must move the file handle into the closure.

The unfortunate thing about this is that it's necessary to wrap the file in Arc. If you look at the definition of async_fs::File, it actually already has an Arc<std::fs::File>. I'm not sure if there is some way we can leverage this implementation detail, or perhaps instead it would be possible to provide a new AsyncFileExt which avoids the Arc by cloning raw file descriptors. I'm not advocating for any particular solution, but it seems there are possibilities.

[al8n]

Ah, I got it. Yes, I will work on this in the near future.

[al8n]

Hi, sorry for the delay; after I review the code, you can use try_lock_exclusive or try_lock_shared to acquire a non-blocking file lock.

[bonsairobo]

@al8n try_lock is not quite what I want because it will just return immediately on failure instead of yielding the task/future.

[al8n]

@al8n try_lock is not quite what I want because it will just return immediately on failure instead of yielding the task/future.

From my point of view, it is hard to make it a "real async task" for locking. We cannot avoid the system call. Even async_fs::File's implementation, for open, write, read, and etc., just use the unblock method to spawn a new task and wait for the task to finish.

Actually, unlike network I/O, you can have great performance improvement using async stuff. For file I/O, there is almost no benefit to using async implementation. This is why databases like redb, sled, and etc use sync File, which may provide some async APIs on the high level.

[bonsairobo]

I'm not trying to make the case that file IO can be faster with async APIs. I'm just saying that async APIs exist for various other reasons, and it would be nice if this crate could cater to that somewhat.

But in any case, I've found a workaround to get unblock working without Arc. I had to move the async_fs::File into the task and then return it back.

[al8n]

Ah, you mean you need some methods like

async fn lock_exclusive(&self) -> std::io::Result<()> {
    ...
}

If so, I think I can add those async methods in a new trait AsyncFileExt

[bonsairobo]

@al8n Yea that would be great!

[al8n]

*_async APIs are now alive in 0.10.0.

[nazar-pc]

The issue with fs4 that there doesn't seem to have any async methods at all. It pretends to have them, but in reality simply blocking runtime internally, which is misleading and confusing.

Tokio does have async file system methods because it uses Arc<File> internally and can clone that Arc and use tokio::task::spawn_blocking into run blocking task on a threadpool.

Unless fs4 does something like that (which I believe it shouldn't do because user can easily do that themselves and for example tokio will not give you direct access to inner Arc<File>), it should remove all fake async methods to prevent issues for downstream users that might naively believe it is actually async and causing problems instead.

[bonsairobo]

I don't agree with the premise that async APIs are harmful, but I agree that the implementation is incorrect. It should be using a thread pool instead of just calling a blocking syscall directly in the async fn.

[nazar-pc]

Async APIs are harmful if they are not in fact async. If they were implemented correctly I'd have nothing against them, but as it is right now, it is deeply problematic.

As explained above, you can't do that as a trait on File easily. You have to make sure file lives as long as the blocking task, even if async future created by the method is dropped. Which is exactly why tokio has a wrapper with Arc<File> inside of it and sends a clone to blocking task. You can't just clone File though, so most of current "async" APIs provided by fs4 are impossible to implement in async way soundly.

[al8n]

To be honest, I am not using any async file implementation when handling file I/O, as I mentioned above, async files almost have no benefits.

a thread pool instead of just calling a blocking syscall directly in the async fn.

It is almost impossible, those methods do not require ownership.

I just thought users should be aware that file locking is not a real "async task", but indeed, as @nazar-pc said, some users may ignore the documentation and naively believe they are actually async.

[nazar-pc]

I think the intentions were good, don't get me wrong, but having an async function that simply does blocking internally helps no one, there is zero benefit to it because anyone can simply call blocking function directly and get literally the same result, but at least they'll know they are blocking the runtime and could use tokio::task::block_in_place to prevent executor blocking.

There are a few options beyond simply removing async methods (which I'd prefer to make library smaller and easier to maintain):

• implement methods on Arc<File> instead of File
• implement methods taking self instead of &self, such that File can be moved in and out of blocking task
• convince maintainers of tokio and other runtimes to expose access to inner Arc<File>, such that you can implement methods for tokio::fs::File correctly

None of the alternative options look particularly appealing to me to be completely honest.

[bonsairobo]

implement methods taking self instead of &self, such that File can be moved in and out of blocking task

This is what I've opted to do in my own use case.

[al8n]

Removing those APIs seems the simplest way, and let users handle this stuff by themselves.

[bonsairobo]

@al8n I'm fine with removing them and punting on a proper async implementation. Sorry if this whole adventure was not ultimately worth your time. I was just hoping to make the crate's API slightly nicer and didn't anticipate this type of issue.