Names module DNS getHostByName error on certain sites

[pythonologist]

With the attached program I get this output:

D:\software\Twisted-trunk\twisted\names\dns.py:1662: DeprecationWarning: Please only pass IPs to write(), not hostnames
  self.transport.write(message.toStr(), address)
result: [Failure instance: Traceback: <type 'exceptions.AttributeError'>: 'NoneType' object has no attribute 'stopListening'
D:\software\Twisted-trunk\twisted\internet\udp.py:126:doRead
D:\software\Twisted-trunk\twisted\names\dns.py:1693:datagramReceived
D:\software\Twisted-trunk\twisted\internet\defer.py:238:callback
D:\software\Twisted-trunk\twisted\internet\defer.py:307:_startRunCallbacks
--- <exception caught here> ---
D:\software\Twisted-trunk\twisted\internet\defer.py:323:_runCallbacks
D:\software\Twisted-trunk\twisted\names\common.py:206:<lambda>
]

Certain domain names appear to return a particular set of DNS records requiring recursion, which does not appear to work properly: one example is www.bbc.co.uk - note that plain bbc.co.uk works fine. Possibly related to #2850.

[pythonologist]

simple dns test program

[exarkun]

I cannot reproduce this. Please attach a DNS traffic capture.

[exarkun]

smcq ran into this error, apparently, and brought it up on IRC. On taking a second look, I was able to reproduce this failure after changing dns_test_2.py to use twistedmatrix.com instead of www.bbc.co.uk. This difference is probably just due to differences in the networks pythonologist and I were running the example on, but I haven't dug into the problem yet so that's just a guess.

I am attaching a pcap file of the dns traffic generated by the script when I run it. Examination (or use, in the worst case) of this should be enough to reproduce the issue in a test case.

[exarkun]

A guess I might make is that #3342 introduced this bug (that would make this a regression :/).

[smcq]

I have tracked this bug to the NS followup query done by extractRecord in common.py.

return r.lookupAddress(str(name)

).addCallback(lambda (ans, auth, add): extractRecord(r, name, ans + auth + add, level - 1) ).addBoth(lambda passthrough: (r.protocol.transport.stopListening(), passthrough)[1])

Simply removing the addBoth (which does appear entirely unnecessary by my reading) will fix the bug.

To duplicate, use a NS record, I was able to successfully duplicate it with saucelabs.com. You may need to set retries to two in order to cause the bug (unsure as to reason, and you may want to dig into that before patching).

With the addBoth removed the following commands successfully run, with it enabled the following commands will cause the server to crash with NoneType not having stopListening.

If you run the following you should be able to reproduce the error against a trivial nameserver implementation:

dig +tries=2 @localhost -p 4053 saucelabs.com

[smcq]

Sorry, I didn't mean to accept this, however I can submit a patch in the morning for my resolution.

[smcq]

Patch that fixes problem. extractRecord incorrectly attempted to close it's resolver's connection.

[jesstess]

Thanks for the patch smcq!

Can you submit a test case as part of this patch? Also, once you're ready for someone to review a patch, please add 'review' to the keywords property and reassign to nobody so it doesn't get lost in the shuffle.

[jesstess]

Oh trac thou dost protesteth too much

[exarkun]

#4210 was a duplicate of this.

[jml]

Twisted 10.1.0 has been released. Removing this ticket from the 10.1 milestone. If you think it's severe enough to demand a follow-up release, please say so and add this ticket back to the 10.1 milestone.

[Scramblejams]

This was a blocker for me, now I have to manually patch all my Twisted installations. And patch them again whenever a security update comes out... Are we just waiting on a test case?

[therve]

(In [29989]) Branching to 'names-no-transport-3998'

[therve]

OK, I did the bare minimum, adding a test failing before and not after. Considering the fix is obvious, I'll it's good enough.

[jml]

Thanks for adding the test therve.

I'm a little confused though as to why the test builds a messages list only to have it returned from query. Wouldn't it be simpler to have query define and return m?

[jml]

<therve> jml, re your comment, I want it to blow up if another query is made
<therve> I guess it doesn't matter that much if the test is passing
<jml> there are eight tickets marked to review, hard to say if they've got patches or not.
<jml> therve, maybe add a comment saying "Blow up if another query is made", as well as saying why you want to do that.
<therve> jml, sounds good

[therve]

Thanks, I added a comment in r29999.

[jml]

Thanks. Please merge.

[therve]

(In [30001]) Merge names-no-transport-3998

Author: therve Reviewer: jml Fixes: #3998

Fix a regression twisted.names.common.extractRecords where it tried to close a non-present transport.