twisted.names.client doesn't close '/etc/resolv.conf'

[duffy151]

in maybeParseConfig(), /etc/resolv.conf is opened but never closed. In PyPy this can lead to out of file descriptor errors until the gc collects them.

[ephess]

Patch with fix uploaded.

Reviewer: Code is already exercised by a unit test and this isn't something I'd write a specific test for normally. Let me know if this approach is OK?

[exarkun]

Thanks for working on this. Generally, all functionality should be unit tested. You're fixing a bug here, which causes real problems on PyPy: anyone who cares about not having these problems will care about there being a test that demonstrates the problems are not re-introduced.

I can see two possible testing approaches for this.

First, hook into the file being opened somehow, so that you can assert that it is also closed at some point. For example, you could add a (private) helper method to client.Resolver for opening files, change the code to use it, and then override it in the unit tests to hand back a StringIO instance which you later check to make sure close has been called on it (somewhat like what is done in twisted.web.test.test_static.StaticProducerTests.test_stopProducingClosesFile).

Second, take advantage of the ResourceWarning which some versions of Python emit when a file is closed by the garbage collector. This involves working on the BuildBot configuration to recognize these warnings as errors and fail a build whenever a new instance is introduced. Then you can fix this problem (which should be causing a ResourceWarning to be emitted) and if anyone ever re-introduces it, BuildBot will report a failed build. This is probably the harder of the two options, and not quite as nice as the first since it won't report problems to developers locally, it will only report them after they trigger a build on BuildBot.

Also, separately, you should add a news fragment for this fix.

Thanks again.

[ephess]

Revised patch addressing concerns raised in first review

[ephess]

Thanks for the feedback exarkun - have gone with the first option. Do you think it would be worth while opening up a second ticket to make the change in Buildbot, as this would also pick up any other parts of the code base that are doing this also?

[glyph]

(In [36732]) Branching to 'close-resolve-conf-6216'

[glyph]

Hi ephess,

There are a couple of minor issues with this patch, but I'll address them quickly and then merge.

1. The standard we use for docstrings in tests is "Foo does bar." and not "Ensure that we verify that foo indeed correctly does bar.". Hopefully that slightly exaggerated example makes it clear why ;-).
2. You should generally avoid patch in tests, since it manipulates global state that either the test framework itself or other tests could be using. If you're going to use patch to test this, you don't actually need to add the _openFile stuff, since you can just patch(twisted.names.client, "FilePath", MyFakeFilePath). If you are going to add the _openFile hook, then you can easily just pass it in rather than having to patch it up. So, in a sense, this test is the worst of both worlds.

Thanks for your contribution!

-glyph

[glyph]

After making those changes and forcing a build, I noticed that my change introduced a  test failure on python 3; I guess we need to use FilePath after all!

[glyph]

(In [36746]) Merge close-resolve-conf-6216

Author: ephess

Reviewer: glyph

Fixes: #6216

Do not depend on the garbage collector to close /etc/resolv.conf when twisted.names.client is done reading it.