Fix #11100 and #8058: binary compatibility and leaked file handle in exec_file()

[mmatrosov]

Fixes the following bugs:
https://svn.boost.org/trac/boost/ticket/11100
https://svn.boost.org/trac/boost/ticket/8058

[stefanseefeld]

Thanks for the patches, these look good.

[stefanseefeld]

The comments appear contradictory: https://svn.boost.org/trac/boost/ticket/8058 reports the issue of non-closed file handles specifically with Python 2.7 on a 64-bit host, while comments above suggest that it's only with Python 2.7 that the file is (attempted to be) closed twice.

[mmatrosov]

@stefanseefeld I encountered the problem described in this ticket under Python 3.4 (32 bit), and tested the fix only under this version. I suggested that this will also fix it under other versions and platforms and I hoped someone will test it. It sounds odd indeed that @reven86 says that behaviour under Python 2.7 is different then described in the ticket.

[stefanseefeld]

@mmatrosov , thanks. For avoidance of doubt: by "the problem" are you referring to the dangling open file descriptor, or to the double free ? I wonder what may cause the file not to be closed via the handle<> destructor to begin with, as that's the typical fail-safe (and exception-safe !) technique used throughout C++.

[mmatrosov]

@stefanseefeld By "the problem" I am referring to the dangling open file descriptor, that's why I added closeit flag in the first place. From the interface I saw no reason why file descriptor should outlive the exec function, that's why I decided to close it immediately. Yes, it was unclear for me, why it was not closing in handle's destructor. But I was not thorough in that fix, perhaps handle's destructor is the right place to close file descriptor in this case. Feel free to implement the solution you find more appropriate.

[stefanseefeld]

@mmatrosov Thanks for the confirmation. Did you, as the OP, observe the open file descriptors accumulating when running program from https://svn.boost.org/trac/boost/ticket/8058 ? Or just after the "exec_file" function (were the file should indeed be still open) ?
Just trying to attempt to reproduce the original failure...
I think I'm going to revert this patch for the upcoming release, even if I can't find a proper fix.

[mmatrosov]

@stefanseefeld I'm sorry, I do not see the difference between the two ("open file descriptors accumulating when running program from ticket 8058" and "just after the "exec_file" function"). What do you mean? Once exec_file is finished, returned handle should be destroyed (unless stored in a variable) and file descriptor should be closed anyway.

[stefanseefeld]

@mmatrosov Sorry, I meant to ask whether you looked at the file after the execution of PyRun_File(), or after exec_file(). But I assume it's the latter.
In any case, the patch is reverted, and I'm waiting to see a report of the file still being open after the handle<> object is destroyed (as the OP seemed to imply). I still don't see how that could ever happen...

[mmatrosov]

@stefanseefeld Interesting. I may prepare a reprocase and send it to you. I used MS Visual Studio 2013, Python 3.4.3, boost 1.57, Windows 7 x64, 32-bit build.

[stefanseefeld]

OK. Please be aware that I don't have Windows here (much less MSVC), so I'm relying on other people testing and debugging on such platforms. So if you could step through the debugger and analyze why the file isn't getting closed, that would be very much appreciated. Thanks !

[mmatrosov]

@stefanseefeld

Please be aware that I don't have Windows here (much less MSVC)

How is boost::python tested on other platforms?

[stefanseefeld]

Different testers use different test platforms, so as long as all supported platforms are covered by some testers, we are fine. Of course, to be able to develop for a specific platform, the developer needs to have access to such a platform. I suppose I could set up a VM with Windows / MSVC on it, but I haven't worked with that platform for ~10 years, so I'd rather have someone else take over that responsibility.

[mmatrosov]

@stefanseefeld Is new functionality tested when it is already merged in master? If there are any specifications of this process (development, merging, testing) specific to boost::python, could you please give me a link to those?

[stefanseefeld]

I'm not aware of any formal documentation of the process (the boost
online docs are quite messy as far as that is concerned, as they still
refer to svn in some places).

There are test runs for both master and the "develop" branch, so the
idea is that all development happens on the "develop" branch, and get
merged to "master" only once things are stable enough. I merged
"develop' to "master" last night after comparing the state of both
(including test results).

http://www.boost.org/development/tests/develop/developer/python.html
http://www.boost.org/development/tests/master/developer/python.html

are the two test reports.

  ...ich hab' noch einen Koffer in Berlin...

[mmatrosov]

Hi, @stefanseefeld. I'm currently investigating the issue. It doesn't seem complicated: we just need to destroy file handle either with help of python::handle<>, or with closeIt flag.

However, now I cannot even make exec_file work under Python 3.4 (everything is ok for Python 2.7). I'm trying to call it like in function exec_file_test() from boost.python\test\exec.cpp:

boost::python::dict global;
exec_file("script.py", global, global);

And for this PyRun_FileEx inside exec_file returns null. Is this a bug? Can you advise me on what to do?

[bchretien]

Hi, I am also observing this bug with Boost 1.58 and Python 2.7 on Arch Linux (in case you need an extra tester for a fix). Any update on the solution?

[stefanseefeld]

I'm not sure what you are referring to as "this bug" here. The one I'm aware of is the leaked file descriptor. For that it would be most useful to come up with a minimal test case, so someone can walk through it in a debugger on one of those platforms that show this behavior, and identify the cause.

[bchretien]

@stefanseefeld I am referring to the bug detailed in the comments of fe24ab9, but maybe this has its own issue on GH and/or was already fixed. The repro code simply involves calling exec_file with Python 2.7 (cf. the example showed here).

[stefanseefeld]

That is fixed in 1.59.

[bchretien]

@stefanseefeld: wonderful, thanks!