Opened 4 years ago

Closed 4 years ago

#6216 defect closed fixed (fixed)

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

Reported by: Kevin Duffy Owned by: Glyph
Priority: normal Milestone:
Component: core Keywords:
Cc: Jordan Hagan, borko Branch: branches/close-resolve-conf-6216
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph


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 Jordan Hagan 4 years ago.
resolvconf-close-6216.2.patch (2.7 KB) - added by Jordan Hagan 4 years ago.
Revised patch addressing concerns raised in first review

Download all attachments as: .zip

Change History (11)

Changed 4 years ago by Jordan Hagan

Attachment: resolvconf-close-6216.patch added

comment:1 Changed 4 years ago by Jordan Hagan

Cc: Jordan Hagan 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 4 years ago by borko

Cc: borko added

comment:3 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jordan Hagan

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 4 years ago by Jordan Hagan

Revised patch addressing concerns raised in first review

comment:4 Changed 4 years ago by Jordan Hagan

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 4 years ago by Jordan Hagan

Owner: Jordan Hagan deleted

comment:6 Changed 4 years ago by Glyph

Author: glyph
Branch: branches/close-resolve-conf-6216

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

comment:7 Changed 4 years 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!


comment:8 Changed 4 years 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 4 years ago by Glyph

Resolution: fixed
Status: newclosed

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