Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Bug #915: Please help

nedbatchelder.com

Post reply on HN
Print view View original

61–70 of 97 posts

Re: Bug #915: Please help

#61

Post by mbreese »

lostmsu wrote: post #42
mmastrac wrote: post #24

Earlier quoted context omitted.

It might be easiest to view it like so: $ echo The shell spawns two commands connected to pipes, then replaces those with a file that represents the other (read) side of that pipe. The command above with >(grep something) does the same thing, just with the other end of a pipe.

Does anybody know if this can be done in Windows (not WSL)? E.g. is it possible to open a named pipe (?) through its name, and work with it as a sequential file?

This isn’t a Linux thing, it’s a bash thing. The A quick search says that Windows has support for named pipes as an IPC mechanism [1], but I’ve never used them. But this is a Windows API. I am not aware for any way to do this with plain cmd.exe. You’re not the first to ask though... [2]

[1] https://docs.microsoft.com/en-us/windows/win32/ipc/named-pip...

[2] https://superuser.com/questions/430466/in-windows-can-i-redi...

Re: Bug #915: Please help

#62

Post by asdfaoeu »

nneonneo wrote: post #48

This doesn't quite get all the way there, but the failure is being caused by the closure of a _io.FileIO due to GC, which happens to have the same fd as the sqlite database. The closure happens in the middle of a SQLite operation, which causes a subsequent flock() call to fail. strace log from the failure: open("/home/travis/apprise-api/.coverage", O_RDWR|O_CREAT|O_CLOEXEC, 0644) = 31 fstat(31, {st_mode=S_IFREG|0644,…

> If you disable this block of code everything runs fine (although the tests fail). It's kinda creepy that the NamedTemporaryFile is leaking - it really shouldn't...

My guess is the tempfile is leaking due to the attribute caching here https://github.com/python/cpython/blob/master/Lib/tempfile.p... . But that doesn't really explain why the file is getting closed twice. (Presuming it's getting closed twice as otherwise the file descriptor wouldn't be reallocated)

Edit: realised it's just attaching the closer object to the function not the full object so avoiding this.

Re: Bug #915: Please help

#63

Post by theli0nheart »

asdfaoeu wrote: post #62
nneonneo wrote: post #48

This doesn't quite get all the way there, but the failure is being caused by the closure of a _io.FileIO due to GC, which happens to have the same fd as the sqlite database. The closure happens in the middle of a SQLite operation, which causes a subsequent flock() call to fail. strace log from the failure: open("/home/travis/apprise-api/.coverage", O_RDWR|O_CREAT|O_CLOEXEC, 0644) = 31 fstat(31, {st_mode=S_IFREG|0644,…

> If you disable this block of code everything runs fine (although the tests fail). It's kinda creepy that the NamedTemporaryFile is leaking - it really shouldn't... My guess is the tempfile is leaking due to the attribute caching here https://github.com/python/cpython/blob/master/Lib/tempfile.p... . But that doesn't really explain why the file is getting closed twice. (Presuming it's getting closed twice as otherwis…

This whole wrapper nonsense to get with working with NamedTemporaryFile seems really suspect in general.

My gut instinct would be to rewrite the section OP found as a standard NamedTemporaryFile instantiation, then close it manually. The __getattr__ hack feels like a real code smell.

Re: Bug #915: Please help

#64

Post by marcan_42 »

nneonneo wrote: post #48

This doesn't quite get all the way there, but the failure is being caused by the closure of a _io.FileIO due to GC, which happens to have the same fd as the sqlite database. The closure happens in the middle of a SQLite operation, which causes a subsequent flock() call to fail. strace log from the failure: open("/home/travis/apprise-api/.coverage", O_RDWR|O_CREAT|O_CLOEXEC, 0644) = 31 fstat(31, {st_mode=S_IFREG|0644,…

Then the problem isn't the fd leaking, the problem is that the fd is being closed twice. If SQLite owns that fd then it's obviously no longer owned by the NamedTemporaryFile, so something closed it. So either NamedTemporaryFile is closing an underlying fd twice (that sounds like a horrible Python bug, it should never be allowed) or some piece of code somewhere did one of those horrible "close all fds from 3 to 1024" things at one point, and SQLite then reused an fd that was still assigned to a live File object.

Re: Bug #915: Please help

#65

Post by nneonneo »

nneonneo wrote: post #48

This doesn't quite get all the way there, but the failure is being caused by the closure of a _io.FileIO due to GC, which happens to have the same fd as the sqlite database. The closure happens in the middle of a SQLite operation, which causes a subsequent flock() call to fail. strace log from the failure: open("/home/travis/apprise-api/.coverage", O_RDWR|O_CREAT|O_CLOEXEC, 0644) = 31 fstat(31, {st_mode=S_IFREG|0644,…

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.

Re: Bug #915: Please help

#66

Post by marcan_42 »

nneonneo wrote: post #65
nneonneo wrote: post #48

This doesn't quite get all the way there, but the failure is being caused by the closure of a _io.FileIO due to GC, which happens to have the same fd as the sqlite database. The closure happens in the middle of a SQLite operation, which causes a subsequent flock() call to fail. strace log from the failure: open("/home/travis/apprise-api/.coverage", O_RDWR|O_CREAT|O_CLOEXEC, 0644) = 31 fstat(31, {st_mode=S_IFREG|0644,…

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 _Temp…

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.

Re: Bug #915: Please help

#67

Post by nemetroid »

nneonneo wrote: post #65
nneonneo wrote: post #48

This doesn't quite get all the way there, but the failure is being caused by the closure of a _io.FileIO due to GC, which happens to have the same fd as the sqlite database. The closure happens in the middle of a SQLite operation, which causes a subsequent flock() call to fail. strace log from the failure: open("/home/travis/apprise-api/.coverage", O_RDWR|O_CREAT|O_CLOEXEC, 0644) = 31 fstat(31, {st_mode=S_IFREG|0644,…

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 _Temp…

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.

Re: Bug #915: Please help

#68

Post by nneonneo »

maxmcd wrote: post #56
nneonneo wrote: post #48

This doesn't quite get all the way there, but the failure is being caused by the closure of a _io.FileIO due to GC, which happens to have the same fd as the sqlite database. The closure happens in the middle of a SQLite operation, which causes a subsequent flock() call to fail. strace log from the failure: open("/home/travis/apprise-api/.coverage", O_RDWR|O_CREAT|O_CLOEXEC, 0644) = 31 fstat(31, {st_mode=S_IFREG|0644,…

Very curious about this wonderful magic. How were you able to get the call stack from the strace line?

I basically set up my GDB with commands to stop on a specific pattern of "lseek, then close", and if the pattern isn't met it just automatically continues the program.

This is what the gdb script looks like:

    set height 0

    catch syscall close
    catch syscall read
    catch syscall lseek
    disable 1 2

    commands 2
     disable 1 2
     continue
    end

    commands 3
     if $rdi == 31
      enable 1 2
      continue
     else
      continue
     end
    end
The lseek catchpoint (3) enables both read and close catchpoints; if the read catchpoint (2) is hit first it disables both and continues. This way we look for lseek followed by close without intervening reads.

It generates a few false positives but otherwise fairly quickly stops on the right syscall, at which point I could backtrace and prod the live program.

Re: Bug #915: Please help

#69

Post by nneonneo »

marcan_42 wrote: post #66
nneonneo wrote: post #65

Earlier quoted context omitted.

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 _Temp…

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…

It's still like that in CPython: 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. 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).

Post reply on HN