Opened 17 years ago
Closed 12 years ago
#970 defect closed fixed (fixed)
twisted.names.root.Resolver eats filedescriptors
Reported by: | jojo | Owned by: | |
---|---|---|---|
Priority: | high | Milestone: | |
Component: | names | Keywords: | |
Cc: | Jean-Paul Calderone, spiv, itamarst, titty, jojo, teratorn, Thijs Triemstra | Branch: |
branches/root-resolver-fd-leak-970-2
branch-diff, diff-cov, branch-cov, buildbot |
Author: | exarkun |
Description
Attachments (2)
Change History (45)
Changed 17 years ago by
comment:2 Changed 17 years ago by
First part of this is fixed and checked in at r13513 and r13514 Dunno what to do about the second part. It seems to be a win32-only problem. It may even have nothing to do with t.names, I can't tell.
comment:3 Changed 16 years ago by
Keywords: | win32 removed |
---|---|
Summary: | [PATCH] Problems with Twisted Names on Windows → twisted.names.root.Resolver eats filedescriptors [Problems with Twisted Names on Windows] |
This isn't win32 only.
Change 'self.resolver = ...' to self.resolver = twisted.names.root.Resolver(192.228.79.201?)
This results in the following error message on linux:
Unhandled error in Deferred: Traceback (most recent call last): File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/internet/defer.py", line 229, in callback File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/internet/defer.py", line 294, in _startRunCallbacks File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/internet/defer.py", line 307, in _runCallbacks File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/internet/defer.py", line 630, in gotResult --- <exception caught here> --- File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/internet/defer.py", line 606, in _deferGenerator File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/names/root.py", line 127, in discoverAuthority File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/names/root.py", line 74, in lookupNameservers File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/names/root.py", line 38, in retry File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/names/dns.py", line 1126, in query File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/names/dns.py", line 1084, in startListening File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/internet/posixbase.py", line 307, in listenUDP File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/internet/udp.py", line 84, in startListening File "/scratch1/ralf/bbot/extern/Twisted-trunk/twisted/internet/udp.py", line 92, in _bindSocket
comment:4 Changed 16 years ago by
While I'm here: Shouldn't the twisted.names.root.bootstrap mechanism be replaced by a static list of IP addresses?
comment:5 Changed 16 years ago by
Ah. It's pretty clear what's going on here now. root.Resolver needs to clean up the resolver it creates for each lookup it does. I'll take care of this shortly.
Regarding bootstrap, what would the advantage of this be? The list would just have to be maintained and updated, and old installs of Twisted would randomly break over time.
What it should actually be replaced (or probably just augmented) with is a platform-specific mechanism for determining the system's actual configured nameservers.
comment:6 Changed 16 years ago by
The advantage is that one can use root.Resolver without first using dns resolving (which might not work).
The platform specific mechanism is working for unix, still root.Resolver isn't used (this is fine). In my opinion this has nothing to do with being able to determine the configured nameservers.
Maybe, it should just be rewritten with a static list of IPs and then using root.Resolver([ips]) to do the name lookups for the root servers. This would also be real bootstrapping :)
comment:7 Changed 16 years ago by
Owner: | changed from itamarst to Jean-Paul Calderone |
---|
comment:8 Changed 15 years ago by
Cc: | teratorn added |
---|
comment:9 Changed 15 years ago by
Priority: | high → low |
---|
Certainly this should still be fixed, but I'm not likely to get to it in the foreseeable future. If someone would like to supply a patch with unit tests, I'll gladly review it, though.
comment:12 Changed 12 years ago by
Summary: | twisted.names.root.Resolver eats filedescriptors [Problems with Twisted Names on Windows] → twisted.names.root.Resolver eats filedescriptors |
---|
comment:13 follow-up: 14 Changed 12 years ago by
Priority: | low → high |
---|
I think this bug has a pretty high priority ..
It shows that might be a design-bug in the twisted.named.root.Resolver()
being able to bootstrap a recursive caching Resolver is very important
at the Moment the root.Resolver() is pretty much useless and should be marked broken
yours
Christian
comment:15 Changed 12 years ago by
i fixed the leaking resolver ports for my use case .. .. though it seems to have problems when the reactor is not running?
please see the attached patchit.txt
yours
Christian Bahls
Changed 12 years ago by
Attachment: | patchit.txt added |
---|
this solves the leaking file descriptors for me ..
comment:16 Changed 12 years ago by
Cc: | Thijs Triemstra added |
---|---|
Keywords: | review added |
Owner: | Jean-Paul Calderone deleted |
comment:17 Changed 12 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jean-Paul Calderone |
Status: | new → assigned |
Needs unit tests badly.
comment:18 Changed 12 years ago by
Author: | → exarkun |
---|---|
Branch: | → branches/root-resolver-fd-leak-970 |
(In [27646]) Branching to 'root-resolver-fd-leak-970'
comment:19 Changed 12 years ago by
Branch: | branches/root-resolver-fd-leak-970 → branches/root-resolver-fd-leak-970-2 |
---|
(In [27694]) Branching to 'root-resolver-fd-leak-970-2'
comment:20 Changed 12 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
Status: | assigned → new |
comment:21 Changed 12 years ago by
Owner: | set to TimAllen |
---|
comment:22 Changed 12 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from TimAllen to Jean-Paul Calderone |
OK, this is my first attempt at a code-review. Hope this works!
t/n/client.py
, line 2: File has been edited, but copyright date not bumped.t/n/client.py
, lines 33-35: You've deleted the_errormap
variable fromResolver
, but the exceptions it contained are still being imported even though they're not used.- It's not immediately obvious why
t.n.dns.RRHeader
has learned how to compare itself to other instances. I mean, it's a useful-looking change, but is it necessary for the rest of the code in this patch to work? t/n/root.py
, line 121: The list of timeouts(1, 3, 11, 45)
appears in nearly a dozen places through the Twisted codebase, and three times inroot.py
alone. Does this sequence come from an RFC or something? Should it be a module-level constant?t/n/root.py
, inResolver
: This code is very confused about whether it wants a single server to query, or a list of them.__init__()
takes a list, which_lookup()
passes to_discoverAuthority()
, which grabs the server at the head of the list and throws the tail away. The head gets passed to_query()
which wraps it back into a list and passes it toclient.Resolver()
. Later on in_discoveredAuthority()
, we take a list of potential servers intraps
, look up only the first one, and wrap it back into a list to pass to_discoverAuthority()
(which will unpack it so_query()
can re-pack it). Since this class is given a list and passes a list to the underlying utility functions, they might as well stay lists all along.t/n/root.py
, line 137: The docstring saystimeout
should be a list of ints, but everywhere that this code supplies a timeout, it's a tuple of ints.t/n/root.py
, line 183: There's a comment that says "Check class too!" If this is just a matter of adding "and answer.class == query.class", you might as well do it.t/n/root.py
, line 206: If we get a response with a list of authoritative name servers and addresses for *some* of them, is it OK to just forward the query to the servers we have addresses for and ignore the others? If we're ignoring information, doesn't that make things less reliable?
comment:23 Changed 12 years ago by
comment:24 Changed 12 years ago by
comment:25 Changed 12 years ago by
comment:27 Changed 12 years ago by
comment:29 Changed 12 years ago by
comment:30 Changed 12 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
Thanks for the in-depth review.
- Date updated
- Unused imports deleted
- This was to ease testing. In a couple places the tests want to verify that the right
RRHeader
is returned. - It's somewhat recommended by some standard, I don't remember which. I think it's an API mistake, but it's hard to fix at this point (I think there's room for a higher-level, nicer API, where perhaps it can be fixed). Since most of the occurrences in root.py are in deprecated code, I'm not too bothered. The net count will go down once that code is deleted.
- Indeed. It keeps lists around in more places now.
- Fixed.
- Added a test for that and implemented the class check.
- That's a pretty good question. I'm not sure what the current best practice is for this. Supposedly, every server included in that response is supposed to be able to answer the query. Of course, in practice, a server might be down, overloaded, misconfigured, etc. It might be sensible to make sure all servers are tried. I think this could easily be a separate unit of work, too, though. And it's pretty reliable as-is.
Beyond what was mentioned in the review, I also implemented some more CNAME logic (following them with an additional query when their target is not included in the response, detecting cycles). I'm not all that happy with how long _discoveredAuthority
is now. The processing logic is somewhat complex, and the implementation reflects this.
comment:31 Changed 12 years ago by
Owner: | set to TimAllen |
---|
comment:32 Changed 12 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from TimAllen to Jean-Paul Calderone |
I did a brief search for the sequence "1, 3, 11, 45", and the only hits are for various copies of the Twisted source across the internet, and a Japanese PDF about DNSSEC. Oh, and it's the first few terms of the sequence of Super Catalan Numbers (they fight crime!). I think it'd be good to have a comment mentioning that these numbers are arbitrary rather than being fundamentally important parts of some DNS RFC, just to ward off cargo-cult programmers (it may be too late for Japanese DNSSEC researchers).
t/n/root.py
, infindAnswerOrCName()
: You say "if record.name == name
". Since therecords
variable was populated by grabbing the.name
attribute from each record, surely that condition should always be true.t/n/root.py
, line 207 infindAnswerOrCName()
: It might be worth pointing out in a comment that a given DNS name should only ever have one CNAME, so it doesn't matter that this method always chooses the last CNAME.t/n/root.py
, in_discoverAuthority()
: I agree, this method is getting unwieldly and long, especially the "while True" loop is a bit mind-bending. On the other hand, I don't have any concrete suggestions on how to improve it either.t/n/root.py
, line 266 in_discoverAuthority()
: If the reply included nohints
, this code assumes it must have included at least one entry intraps
, which seems a bold assertion (there's got to be *some* sufficiently misconfigured DNS server out there somewhere). In this circumstance, raising some kind of 'DNS server is stupid' error would be more helpful thanIndexError: list index out of range
. Likewise for assuming that the first entry intraps
resolves to at least one sensible IP address.t/n/t/test_rootresolve.py
, in_getResolver
docstring: The "@param" line needs an extra space of indent.
comment:34 Changed 12 years ago by
comment:35 Changed 12 years ago by
comment:36 Changed 12 years ago by
comment:37 Changed 12 years ago by
comment:40 Changed 12 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
Thanks! All points addressed.
comment:41 Changed 12 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jean-Paul Calderone |
Looks good to me! Ready to merge, as far as I can see.
comment:42 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [27740]) Merge root-resolver-fd-leak-970-2
Author: exarkun Reviewer: TimAllen Fixes: #970
Fix twisted.names.root.Resolver
so that it does not leak several file descriptors
(udp ports) for each lookup it performs. Also re-introduce tests for this code and
deprecate several of the previous APIs which were really only implementation details,
not functionality intended to be made public.
comment:43 Changed 11 years ago by
Owner: | Jean-Paul Calderone deleted |
---|