Opened 10 years ago

Closed 10 years ago

#4540 defect closed fixed (fixed)

t.n.hosts.searchFileFor does not close the file

Reported by: fijal Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: fijal Branch: branches/close-hosts-4540
branch-diff, diff-cov, branch-cov, buildbot
Author: fijal

Description

Which leads to -1 EMFILE (Too many open files) on non-refcounting based python implementations (like PyPy).

Attachments (3)

t.n.hosts.diff (1.1 KB) - added by fijal 10 years ago.
Patch
out.diff (1.9 KB) - added by fijal 10 years ago.
out.2.diff (2.1 KB) - added by fijal 10 years ago.

Download all attachments as: .zip

Change History (17)

Changed 10 years ago by fijal

Attachment: t.n.hosts.diff added

Patch

comment:1 Changed 10 years ago by fijal

Keywords: review added
Owner: Glyph deleted

comment:2 Changed 10 years ago by fijal

Cc: fijal added

comment:3 Changed 10 years ago by Michael Hudson-Doyle

Keywords: review removed

Boringly, ReviewProcess requires that this patch adds tests and API docs for searchFileFor. Neither should be very hard though.

It also needs a file describing the change in twisted/names/topfiles/4540.bugfix. This should be really easy :-)

Changed 10 years ago by fijal

Attachment: out.diff added

comment:4 Changed 10 years ago by fijal

Keywords: review added

comment:5 Changed 10 years ago by Jonathan Jacobs

Keywords: review removed
Owner: set to fijal
  1. The docstring for searchFileFor has a few issues:
    1. Docstrings should have the triple-quotes each on a line of their own.
    2. The grammar in docstring searchFileFor could probably be clearer about the fact that file should be in hosts(5) standard form.
    3. It looks like "give" should be "given".
  2. test_searchFileFor needs a docstring.
  3. Can you make sure there are 3 newlines before FakeDNSDatagramProtocol in test_names.py?

comment:6 Changed 10 years ago by Jonathan Jacobs

I forgot to mention: There should be tests for the cases where searchFileFor returns None.

Also, it looks like there are a bunch of other whitespace issues (mostly the number of newlines between classes and methods), refer to <http://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html>.

Changed 10 years ago by fijal

Attachment: out.2.diff added

comment:7 Changed 10 years ago by fijal

Keywords: review added
Owner: fijal deleted

Fixed reviewer notes (except whitespace a bit).

comment:8 Changed 10 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/close-hosts-4540

(In [29588]) Branching to 'close-hosts-4540'

comment:9 Changed 10 years ago by Jean-Paul Calderone

(In [29589]) Apply out.2.diff

refs #4540

comment:10 Changed 10 years ago by Jean-Paul Calderone

(In [29590]) add news fragment too

refs #4540

comment:11 Changed 10 years ago by Jean-Paul Calderone

Author: exarkunfijal

comment:12 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jean-Paul Calderone
Status: newassigned

I fixed some minor issues with the patch, whitespace, copyright dates, docstrings, and an assert method (r29590, r29591, r29592, r29593). The rest seems fine to me and the build results are good, merging.

comment:13 Changed 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: assignedclosed

(In [29594]) Merge close-hosts-4540

Author: fijal, exarkun Reviewer: mwh, jonathanj, exarkun Fixes: #4540

Explicitly close the hosts file in twisted.names.hosts.searchFileFor, rather than relying on reference counting to do it for us in a timely manner. This improves behavior on PyPy and other runtimes without reference counting.

comment:14 Changed 9 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.