Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSError opening same fd twice #997

Closed
carlmontanari opened this issue Apr 8, 2024 · 7 comments · Fixed by #1000
Closed

OSError opening same fd twice #997

carlmontanari opened this issue Apr 8, 2024 · 7 comments · Fixed by #1000

Comments

@carlmontanari
Copy link
Contributor

Describe the bug
Describe the observed behavior, and what you expect to happen instead.
In case of a crash, please provide the complete stack trace.

👋 in the recent release there were some changes around opening fake files and checking their permissions in #967 which resulted in #983 it looks like.

I have a situation (repro below) where I open the same fd twice and create an io.BufferedRWPair -- with this latest update this fails with the below exception as the file is already opened once.

To be honest im not sure this is a bug or if what I have is not very sane/smart -- the relevant bits for me where vendor'd from pexepct/ptyprocess and ive pretty much left it alone for forever :)

What is/was the reason for only allowing things that aren't pypy to open a file once?

        newline, open_modes = self._handle_file_mode(mode, newline, open_modes)
        opened_as_fd = isinstance(file_, int)
        if opened_as_fd and not helpers.IS_PYPY:
            fd: int = cast(int, file_)
            # cannot open the same file descriptor twice, except in PyPy
            for f in self.filesystem.get_open_files(fd):
                if isinstance(f, FakeFileWrapper) and f.opened_as_fd:
>                   raise OSError(errno.EBADF, "Bad file descriptor")
E                   OSError: [Errno 9] Bad file descriptor

venv/lib/python3.11/site-packages/pyfakefs/fake_open.py:152: OSError

How To Reproduce
Please provide a unit test or a minimal reproducible example that shows
the problem.

from io import BufferedRWPair
from pyfakefs.fake_filesystem_unittest import Patcher


def test_repro():
    with Patcher(use_cache=False) as patcher:
        fs_ = patcher.fs

        fs_.create_file("dummy")
        dummy_file = open("/dummy")

        fd = dummy_file.fileno()

        readf = open(fd, "rb", buffering=0)
        writef = open(fd, "wb", buffering=0, closefd=False)
        _ = BufferedRWPair(readf, writef)

Your environment
Please run the following in the environment where the problem happened and
paste the output.

❯ python -c "import platform; print(platform.platform())"
macOS-14.4.1-arm64-arm-64bit
❯ python -c "import sys; print('Python', sys.version)"
Python 3.11.9 (main, Apr  2 2024, 08:25:04) [Clang 15.0.0 (clang-1500.3.9.4)]
❯ python -c "from pyfakefs import __version__; print('pyfakefs', __version__)"
pyfakefs 5.4.0
❯ python -c "import pytest; print('pytest', pytest.__version__)"
pytest 7.4.4

Thanks a bunch for this project, been using it in testing for a long time and its always been so nice and easy to work with!

carl

@mrbean-bremen
Copy link
Member

Thanks! I somewhat expected new issues to crop up with that change, as the tests cover only a small part of the possible usages...

What is/was the reason for only allowing things that aren't pypy to open a file once?

That was just an observation of the behavior in the real file system, at least with the tests I ran. Pypi often behave slightly different compared to CPython - I did not investigate the cause.

You probably have to pin pyfakefs to the previous version as long as this is not fixed. I'm not sure about the timeline for the fix, these issues tend to be a bit tricky - I will let you know as soon as I understand the problem.

@mrbean-bremen
Copy link
Member

Hm, I'm having trouble understanding the behavior in the real file system. If I create a file my_file, and do:

    fd = my_file.fileno()
    readf = open(fd, "rb", buffering=0)
    writef = open(fd, "wb", buffering=0, closefd=False)
    os.close(fd)

this works. If I do instead:

    fd = my_file.fileno()
    open(fd, "rb", buffering=0)
    open(fd, "wb", buffering=0, closefd=False)
    os.close(fd)

e.g. the same, but not assigning the created file to a variable, I get an OSError (invalid file handle) on the second open call. This is the behavior, that pyfakefs simulates and tests.

I'm just recording this for reference, I'm a bit stumped at the moment...

@mrbean-bremen
Copy link
Member

After writing this, I realized that this means that the file is closed if it is not assigned as in the second case, which explains the invalid file descriptor. This explains the behavior, though I will have to rethink the logic here - I'm not sure how to correctly implement this. This may take some time...

@mrbean-bremen
Copy link
Member

So, to sum this up: I will remove the ugly code that caused this and reopen the issue which I had fixed by that.

Now that I understand that this is a reference counting / garbage collection problem, it is also clear why the behavior is different in pypy. The file is closed if the file object is released, which only can happen immediately after the line it was created, if the file is a C object. With a Python implementation (pypy) this will happen later when the garbage collector kicks in.

I have no idea yet what to do about that other issue without adding C code to pyfakefs (which I really don't want to), but that is something unrelated to this issue.

@mrbean-bremen
Copy link
Member

mrbean-bremen commented Apr 9, 2024

Actually I don't have to reopen the other issue, as this specific behavior was not a part of the issue.
Looks I found this myself while testing, and misunderstood the behavior. The case that it was supposed to fix is probably not relevant, as the code makes no real sense, so I will just ignore it. It was all just a tempest in a teacup... (well, mostly).

@mrbean-bremen
Copy link
Member

@carlmontanari - I'm going to handle another issue first, and make a new release after that.

@mrbean-bremen
Copy link
Member

The other issue may take a bit longer, so I decided to make a patch release now - done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants