3

TL;DR: The Standard Library fails to close a file when an exception is raised. I'm looking for the best way to handle this situation. Feel free to read from the paragraph beginning with "Upon closer inspection of CPython's source code". Also scroll down to the end of the question to grab a self-contained script that reproduces this issue on Windows.

I'm writing a Python package in which I use STL's ConfigParser (2.x) or configparser (3.x) to parse user config file (I will refer to both as ConfigParser since the problem mainly lies in the 2.x implementation). From now on my relevant lines of code on GitHub will be linked when appropriate throughout. ConfigParser.ConfigParser.read(filenames) (used in my code here) raises a ConfigParser.Error exception when the config file is malformed. I have some code in my test suite targeting this situation, using unittest.TestCase.assertRaises(ConfigParser.Error). The malformed config file is properly generated with tempfile.mkstemp (the returned fd is closed with os.close before anything) and I attempt to remove the temp file with os.remove.

The os.remove is where trouble begins. My tests fail on Windows (while working on both OS X and Ubuntu) with Python 2.7 (see this AppVeyor build):

Traceback (most recent call last): File "C:\projects\storyboard\tests\test_util.py", line 235, in test_option_reader os.remove(malformed_conf_file) WindowsError: [Error 32] The process cannot access the file because it is being used by another process: 'c:\\users\\appveyor\\appdata\\local\\temp\\1\\storyboard-test-3clysg.conf'

Note that as I said above, malformed_conf_file is generated with tempfile.mkstemp and immediately closed with os.close, so the only time it is opened is when I call ConfigParser.ConfigParser.read([malformed_conf_file]) here inside the unittest.TestCase.assertRaises(ConfigParser.Error) context. So the culprit seems to be the STL rather than my own code.

Upon closer inspection of CPython's source code, I found that ConfigParser.ConfigPaser.read indeed doesn't close the file properly when an exception is raised. The read method from 2.7 (here on CPython's Mercurial) has the following lines:

for filename in filenames: try: fp = open(filename) except IOError: continue self._read(fp, filename) fp.close() read_ok.append(filename)

The exception (if any) is raised by self._read(fp, filename), but as you can see, if self._read raises, then fp won't be closed, since fp.close() is only called after self._read returns.

Meanwhile, the read method from 3.4 (here) does not suffer from the same problem since this time they properly embedded file handling in a context:

for filename in filenames: try: with open(filename, encoding=encoding) as fp: self._read(fp, filename) except OSError: continue read_ok.append(filename)

So I think it's pretty clear that the problem is a defect in 2.7's STL. And what's the best way to handle this situation? Specifically:

  • Is there anything I can do from my side to close that file?
  • Is it worth reporting to bugs.python.org?

For now I guess I'll just add a try .. except OSError .. to that os.remove (any suggestions?).

Update: A self-contained script that can be used to reproduce this issue on Windows:

#!/usr/bin/env python2.7 import ConfigParser import os import tempfile def main(): fd, path = tempfile.mkstemp() os.close(fd) with open(path, 'w') as f: f.write("malformed\n") config = ConfigParser.ConfigParser() try: config.read(path) except ConfigParser.Error: pass os.remove(path) if __name__ == '__main__': main()

When I run it with Python 2.7 interpreter:

Traceback (most recent call last): File ".\example.py", line 19, in <module> main() File ".\example.py", line 16, in main os.remove(path) WindowsError: [Error 32] The process cannot access the file because it is being used by another process: 'c:\\users\\redacted\\appdata\\local\\temp\\tmp07yq2v'
Improve this question
8
  • Although the 2.x isn't using a context manager (for backwards compatibility), the file should be closed when the handle goes out of scope (see e.g. stackoverflow.com/a/2404671/3001761) - do you still have a reference to it? Could you provide a minimal example to allow others to recreate the issue without cloning your whole repo? Commented Apr 26, 2015 at 19:04
  • @jonrsharpe Hmm, good point. Then do you have any idea why that WindowError is raised? Commented Apr 26, 2015 at 19:05
  • @jonrsharpe except that the traceback object can still hold a reference to the file handle - isn't this exactly what's happening here? Commented Apr 26, 2015 at 19:07
  • Can you provide a self-contained example demonstrating the problem? It's difficult to debug this without having a file that will fail in the particular way you're describing. Commented Apr 26, 2015 at 19:08
  • 1
    @BrenBarn Updated with example. Thanks for the suggestion. Commented Apr 26, 2015 at 19:20

1 Answer 1

Reset to default
3

This is an interesting problem. As Lukas Graf noted in a comment, the problem seems to be that the exception traceback object holds a reference to the call frame where the exception was raised. This call frame includes the local variables that existed at that time, one of which is a reference to the open file. So that file object still has a reference to it and is not properly closed.

For your self-contained example, simply removing the try/except ConfigParser.Error "works": the exception about the malformed config file is uncaught and stops the program. However, in your actual application, assertRaises is catching the exception in order to check that it is the one you're testing for. I'm not 100% sure why the traceback persists even after the with assertRaises block, but apparently it does.

For your example, another more promising fix is to change the pass in your except clause to sys.exc_clear():

try: config.read(path) except ConfigParser.Error: sys.exc_clear()

This will get rid of the pesky traceback object and allow the file to be closed.

However, it's not clear exactly how to do that in your real application, because the offending except clause there is inside unittest. I think the easiest thing might be to not use assertRaises directly. Instead, write a helper function that does your test, checks for the exception you want, cleans up with the sys.exc_clear() trick, and then raises another custom exception. Then wrap a call to that helper method in assertRaises. This way you gain control of the problematic exception raised by ConfigParser and can properly clean it up (which unittest isn't doing).

Here's a sketch of what I mean:

# in your test method assertRaises(CleanedUpConfigError, helperMethod, conf_file, malformed_conf_file) # helper method that you add to your test case class def helperMethod(self, conf_file, malformed_conf_file): gotRightError = False try: or6 = OptionReader( config_files=[conf_file, malformed_conf_file], section='sec', ) except ConfigParser.Error: gotRightError = True sys.exc_clear() if gotRightError: raise CleanedUpConfigError("Config error was raised and cleaned up!")

I haven't actually tested this, of course, because I don't have the whole unittest thing set up with your code. You might have to tweak it a bit. (Come to think of it, you might not even need exc_clear() if you do this, because since the exception handler is now in a separate function, the traceback should be correctly cleared when helperMethod exits.) However, I think this idea may get you somewhere. Basically you need to make sure the except clause that catches this particular ConfigParser.Error is written by you, so that it can be cleaned up before you try to remove your test file.

Addendum: it seems that if the context manager handles an exception, the traceback is actually stored until the function containing the with block ends, as this example shows:

class CM(object): def __init__(self): pass def __enter__(self): return self def __exit__(self, exc_type, exc_value, tb): return True def foo(): with CM(): raise ValueError print(sys.exc_info())

Even though the with block has ended by the time the print occurs, so the exception handling should be finished, sys.exc_info still returns exception info as if there were an active exception. This is what is happening in your code too: the with assertRaises block causes the traceback to persist all the way to the end of that function, interfering with your os.remove. This seems like buggy behavior, and I notice that it no longer works this way in Python 3 (the print prints (None, None None)), so I imagine this was a wart that was fixed with Python 3.

Based on this, I suspect it may be sufficient to just insert a sys.exc_clear() right before your os.remove (after the with assertRaises block).

Improve this answer
Sign up to request clarification or add additional context in comments.

8 Comments

Thanks, reading your answer right now. As to "why the traceback persists even after the with assertRaises block, but apparently it is", I believe the reason is that the contextmanager itself has an exception attribute that holds the caught exception so that you can inspect further. I'm not very familiar with GC process but I guess the contextmanager isn't deleted until end of scope, even if it is named as in with ... as .... See the docs of assertRaises.
@4ae1e1: At first that's what I thought, but the exception object is different from the traceback object. The exception object iself doesn't contain all the references to stack frames, so just storing that shouldn't matter.
Hmm, looking at the source code of ConfigParser.ConfigParser._read, the exception objects only contain fpname and possibly the line and line numbers, so as you said the file object fp shouldn't in an exception object. Confused.
@4ae1e1: I added an addendum to my answer. It appears that if the context manager handles an exception, all the exception info (including the traceback) persists until the end of the function containing the with block.
Thank you for the sys.exc_clear trick and the detailed explanation of exception handling within context manager! I learned quite a bit. I upvoted the answer, but not accepting it just yet to see if there are any better solutions (I doubt it).
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.