#6216 defect closed fixed (fixed)

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

Reported by: duffy151 Owned by: glyph
Priority: normal Milestone:
Component: core Keywords:
Cc: fs@…, borko84@… Branch: branches/close-resolve-conf-6216
(diff, github, buildbot, log)
Author: glyph Launchpad Bug:

Description

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.

Attachments (2)

resolvconf-close-6216.patch (493 bytes) - added by ephess 20 months ago.
resolvconf-close-6216.2.patch (2.7 KB) - added by ephess 19 months ago.
Revised patch addressing concerns raised in first review

Download all attachments as: .zip

Change History (11)

Changed 20 months ago by ephess

comment:1 Changed 20 months ago by ephess

  • Cc fs@… added
  • Keywords review added

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?

comment:2 Changed 20 months ago by borko

  • Cc borko84@… added

comment:3 Changed 20 months ago by exarkun

  • Keywords review removed
  • Owner set to ephess

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.

Changed 19 months ago by ephess

Revised patch addressing concerns raised in first review

comment:4 Changed 19 months ago by ephess

  • Keywords review added

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?

comment:5 Changed 19 months ago by ephess

  • Owner ephess deleted

comment:6 Changed 19 months ago by glyph

  • Author set to glyph
  • Branch set to branches/close-resolve-conf-6216

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

comment:7 Changed 19 months ago by glyph

  • Keywords review removed
  • Owner set to 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

comment:8 Changed 19 months ago by 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!

comment:9 Changed 19 months ago by glyph

  • Resolution set to fixed
  • Status changed from new to closed

(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.

Note: See TracTickets for help on using tickets.