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

Remove safe_text_dupfile() and simplify EncodedFile #6899

Merged
merged 1 commit into from
Mar 14, 2020

Conversation

bluetech
Copy link
Member

I tried to understand what the safe_text_dupfile() function and EncodedFile class do. Outside tests, EncodedFile is only used by safe_text_dupfile, and safe_text_dupfile is only used by FDCaptureBinary.__init__(). I then started to eliminate always-true conditions based on the single call site, and in the end nothing was left except of a couple workarounds that are still needed.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for clearing this up. I also don't fully understand the code you have removed, but given that it is very old (and probably meant to support Python versions that are not supported anymore) and all tests (except one of the jobs) pass, I believe the changes are good.

Just need to figure out the failing job then. 👍

@bluetech
Copy link
Member Author

Thanks @nicoddemus.

The failing test is

    def test_crash_on_closing_tmpfile_py27(testdir):
        p = testdir.makepyfile(
            """
            import threading
            import sys
    
            printing = threading.Event()
    
            def spam():
                f = sys.stderr
                print('SPAMBEFORE', end='', file=f)
                printing.set()
    
                while True:
                    try:
                        f.flush()
                    except (OSError, ValueError):
                        break

             def test_spam_in_thread():
                t = threading.Thread(target=spam)
                t.daemon = True
                t.start()
    
                printing.wait()
        """
        )
        # Do not consider plugins like hypothesis, which might output to stderr.
        testdir.monkeypatch.setenv("PYTEST_DISABLE_PLUGIN_AUTOLOAD", "1")
        result = testdir.runpytest_subprocess(str(p))
        assert result.ret == 0
>       assert result.stderr.str() == ""
E       AssertionError: assert 'SPAMBEFORE' == ''
E         + SPAMBEFORE

My best guess is that the test is racy in that the thread might switch after the printing.set() but before the f.flush(). Then the main thread switches the stderr fd back to the real one, and the flush happens on the real stderr, and the SPAMBEFORE "escapes". Maybe this PR made it more likely?

I will look into it later. Mostly I need to figure out what this test is testing exactly.

I tried to understand what the `safe_text_dupfile()` function and
`EncodedFile` class do. Outside tests, `EncodedFile` is only used by
`safe_text_dupfile`, and `safe_text_dupfile` is only used by
`FDCaptureBinary.__init__()`. I then started to eliminate always-true
conditions based on the single call site, and in the end nothing was
left except of a couple workarounds that are still needed.
@bluetech
Copy link
Member Author

I added write_through=True, to disable all buffering, and the CI passes now, so I think it's good to merge. There's a chance the test is just flaky.

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

Successfully merging this pull request may close these issues.

2 participants