Race condition in `AIOFile.open()`

[h4l]

AIOFile.open() does not prevent concurrent open() calls from each opening the file. As a result, the following scenario can occur:

• An AIOFile object is opened by task A, resulting in FD (file descriptor) A
• Meanwhile, task B calls open()
• Task A starts an operation on the AIOFile object using FD A.
• Task B's open() call completes, overwriting the AIOFile._file_obj created by task A's open() call, causing it to be GC'd and FD A to be closed
• Task A's operation using FD A now fails because FD A is closed

This program reproduces this behaviour:

import asyncio
import os
import tempfile

from aiofile import AIOFile


async def use_file(label: str, af: AIOFile) -> None:
    fd = await af.open()
    if fd is None:
        print(f"{label}: file already open")
    else:
        print(f"{label}: opened file: {fd=}")
    try:
        await af.fsync()  # could be anything
        print(f"{label}: fsync() succeeded")
    except Exception as error:
        print(
            f"{label}: fsync() failed: opened fd={fd}, current AIOFile fd={af.fileno()}, {error=}"
        )


async def main():
    fd, name = tempfile.mkstemp()
    os.close(fd)
    af = AIOFile(name)
    await asyncio.gather(*(use_file(f"task {n}", af) for n in range(10)))


if __name__ == "__main__":
    asyncio.run(main())

$ python aiofile_open_bug.py
task 0: opened file: fd=8
task 1: opened file: fd=9
task 2: opened file: fd=10
task 3: opened file: fd=11
task 5: opened file: fd=13
task 7: opened file: fd=15
task 4: opened file: fd=12
task 0: fsync() failed: opened fd=8, current AIOFile fd=12, error=OSError(9, 'Bad file descriptor')
task 6: opened file: fd=14
task 8: opened file: fd=8
task 9: opened file: fd=16
task 1: fsync() failed: opened fd=9, current AIOFile fd=16, error=OSError(9, 'Bad file descriptor')
task 2: fsync() failed: opened fd=10, current AIOFile fd=16, error=OSError(9, 'Bad file descriptor')
task 3: fsync() failed: opened fd=11, current AIOFile fd=16, error=OSError(9, 'Bad file descriptor')
task 5: fsync() failed: opened fd=13, current AIOFile fd=16, error=OSError(9, 'Bad file descriptor')
task 7: fsync() failed: opened fd=15, current AIOFile fd=16, error=OSError(9, 'Bad file descriptor')
task 4: fsync() failed: opened fd=12, current AIOFile fd=16, error=OSError(9, 'Bad file descriptor')
task 6: fsync() failed: opened fd=14, current AIOFile fd=16, error=OSError(9, 'Bad file descriptor')
task 8: fsync() failed: opened fd=8, current AIOFile fd=16, error=OSError(9, 'Bad file descriptor')
task 9: fsync() succeeded

[mosquito]

I intentionally didn't put any locks on it. Here the behavior completely repeats the behavior of the operating system. Usually the user has to take care of opening and closing the file correctly, tell us why you want to open the same instance in different coroutines?

[h4l]

I use the AIOFile in a class. Its methods which operate on the file assume it may not have been opened, so they await the AIOFile object before using it to ensure it's open. I don't think there's a way to check if an AIOFile was already opened (without poking at the private _file_obj) so my __init__() can't enforce that the file is open when instances are created. And if I opened it in init myself, I'd have to create an asyncio.Task for the async open to handle failures.

I will probably refactor to do the open once in my __aenter__(). I guess I don't really want to open it multiple times, but doing so was the simplest way to ensure it was open, and I didn't expect that doing that would cause this kind of bug.

[h4l]

Are you happy with the current behaviour, or would you be open to a PR to change it? A few options come to mind:

1. Use a lock so that concurrent open() calls can't result in the actual file being opened more than once
2. Don't lock, but take an optimistic concurrency control approach — after getting a file handle, check again if one was opened while waiting, and don't overwrite the existing FD if so
3. As 2. but raise an error if the file was concurrently opened more than once (assuming you feel it's not a valid use of the API to call open() concurrently)
4. Something else?

I'd be happy to contribute a PR.

[mosquito]

@h4l I think should trying to add asyncio.Lock. Of course create a PR, I will assist to fix it.