4 ms·

OK, I found the root cause. The test harness uses mock to cause NamedTemporaryFile to fail in a few places: with patch('tempfile._TemporaryFileWrapper

OK, I found the root cause. The test harness uses mock to cause NamedTemporaryFile to fail in a few places: with patch('tempfile._TemporaryFileWrapper') as mock_ntf: mock_ntf.side_effect = OSError() Unfortunately tempfile.NamedTemporaryFile is not properly set up to detect failures from tempfile._TemporaryFileWrapper: try: file = _io.open(fd, mode, buffering=buffering, newline=newline, encoding=encoding) return _TemporaryFileWrapper(file, name, delete) except BaseException: _os.unlink(name) _os.close(fd) raise Notice how `file` itself isn't cleaned up in the event that _TemporaryFileWrapper throws; instead, the underlying `fd` is closed directly. If `file` was actually opened correctly, it will now dangle, and some time later `file` will get GCed, attempt to close its fd, and fireworks ensue. I don't know who's fault this ultimately is, but it does illustrate a possible danger of mock testing when messing with the internals of other libraries.

marcan_42 7y ago
That would do it. Calling _os.close() on an fd that is now owned by a file object is a big no-no. Both pieces of code are broken. "except BaseException" is wrong because it'll catch KeyboardInterrupt and such, after the file object exists. The mock is wrong because it's introducing an exception that the original code can never raise and was written to assume wouldn't be raised, thus turning a corner case rare bug (fd reuse when a user ^Cs a program) into a bigger problem. The tempfile code should change to only catch Exceptions, not BaseExceptions; it may opt to additionally catch all BaseExceptions and only unlink the name, but that's probably not sufficient anyway since a KeyboardInterrupt may be raised before the whole try block. Really, the unlink should happen in a wider scope try block, and the _os.close should be more tightly constrained to the _io.open call. And the exception matching should be narrower, because it's safer to leak an fd than to accidentally close it twice. Then either the test harness should find some other way to implement that, or they can ask the tempfile people to move the _TemporaryFileWrapper out of the try block so they can keep using that hook point, if it hasn't been already by that point.
- nneonneo 7y ago
  It's still like that in CPython: https://github.com/python/cpython/blob/master/Lib/tempfile.py#L542 https://github.com/python/cpython/blob/master/Lib/tempfile.p... If there's a strong enough case that this is broken (and the KeyboardInterrupt example is a good one), I think we should file a bug about it. EDIT: Created issue39318: https://bugs.python.org/issue39318 https://bugs.python.org/issue39318. I think the possibility of this failing due to a KeyboardInterrupt, even if remote, is sufficient to consider this a bug (and with KeyboardInterrupt, it would be extremely subtle and hard to reproduce - more reason to get it fixed).
  - avip 7y ago
    Impressive debugging expertise! relevant history: https://bugs.python.org/issue21058 https://bugs.python.org/issue21058 https://bugs.python.org/issue26385 https://bugs.python.org/issue26385
    - nneonneo 7y ago
      Wacky: the patch given in the initial submission of issue21058 is correct, but the patch that was actually committed incorrectly placed the wrapper inside the try block.
      - mixmastamyk 7y ago
        Interesting, that bug/patch was written by a guy I worked with at weta. He was always pushing code past its breaking point. I remember reading this bug before. Looks like his fix didn’t have this bug. It may have been modified later, his patch was from 2014.
nemetroid 7y ago
Very nice work. > I don't know who's fault this ultimately is, but it does illustrate a possible danger of mock testing when messing with the internals of other libraries. It doesn't look like there's any way that _TemporaryFileWrapper() could throw an OSError. I'd consider that mock a bug.
- ehsankia 7y ago
  It's also why you keep your try/catch blocks as small as possible. In this case, if the return was outside, which it probably should've been, then the error would've bubbled up instead of causing I/O failure.
Cyphase 7y ago
Here's Ned's blog post describing the solution: https://nedbatchelder.com/blog/202001/bug_915_solved.html https://nedbatchelder.com/blog/202001/bug_915_solved.html
mrtweetyhack 7y ago
What did he win?

marcan_42 7y ago

That would do it. Calling _os.close() on an fd that is now owned by a file object is a big no-no. Both pieces of code are broken. "except BaseException" is wrong because it'll catch KeyboardInterrupt and such, after the file object exists. The mock is wrong because it's introducing an exception that the original code can never raise and was written to assume wouldn't be raised, thus turning a corner case rare bug (fd reuse when a user ^Cs a program) into a bigger problem. The tempfile code should change to only catch Exceptions, not BaseExceptions; it may opt to additionally catch all BaseExceptions and only unlink the name, but that's probably not sufficient anyway since a KeyboardInterrupt may be raised before the whole try block. Really, the unlink should happen in a wider scope try block, and the _os.close should be more tightly constrained to the _io.open call. And the exception matching should be narrower, because it's safer to leak an fd than to accidentally close it twice. Then either the test harness should find some other way to implement that, or they can ask the tempfile people to move the _TemporaryFileWrapper out of the try block so they can keep using that hook point, if it hasn't been already by that point.

nneonneo 7y ago
It's still like that in CPython: https://github.com/python/cpython/blob/master/Lib/tempfile.py#L542 https://github.com/python/cpython/blob/master/Lib/tempfile.p... If there's a strong enough case that this is broken (and the KeyboardInterrupt example is a good one), I think we should file a bug about it. EDIT: Created issue39318: https://bugs.python.org/issue39318 https://bugs.python.org/issue39318. I think the possibility of this failing due to a KeyboardInterrupt, even if remote, is sufficient to consider this a bug (and with KeyboardInterrupt, it would be extremely subtle and hard to reproduce - more reason to get it fixed).
- avip 7y ago
  Impressive debugging expertise! relevant history: https://bugs.python.org/issue21058 https://bugs.python.org/issue21058 https://bugs.python.org/issue26385 https://bugs.python.org/issue26385
  - nneonneo 7y ago
    Wacky: the patch given in the initial submission of issue21058 is correct, but the patch that was actually committed incorrectly placed the wrapper inside the try block.
    - mixmastamyk 7y ago
      Interesting, that bug/patch was written by a guy I worked with at weta. He was always pushing code past its breaking point. I remember reading this bug before. Looks like his fix didn’t have this bug. It may have been modified later, his patch was from 2014.

nemetroid 7y ago

Very nice work. > I don't know who's fault this ultimately is, but it does illustrate a possible danger of mock testing when messing with the internals of other libraries. It doesn't look like there's any way that _TemporaryFileWrapper() could throw an OSError. I'd consider that mock a bug.

ehsankia 7y ago
It's also why you keep your try/catch blocks as small as possible. In this case, if the return was outside, which it probably should've been, then the error would've bubbled up instead of causing I/O failure.

Cyphase 7y ago

Here's Ned's blog post describing the solution: https://nedbatchelder.com/blog/202001/bug_915_solved.html https://nedbatchelder.com/blog/202001/bug_915_solved.html

mrtweetyhack 7y ago

What did he win?