Opened 10 years ago

Closed 9 years ago

#2492 defect closed fixed (fixed)

Unhandled socket error when making DNS query

Reported by: p1mrx Owned by:
Priority: highest Milestone:
Component: names Keywords:
Cc: therve, Jonathan Lange Branch:
Author:

Description (last modified by therve)

A user of a program that I'm writing reported this error. It occurs during "self.writeMessage(m, address)", and I think twisted should be catching this and calling the errback instead of letting the exception make its way back to my code.

Traceback (most recent call last):
 File "dtella.py", line 446, in <module>
 File "dtella.py", line 419, in run
 File "twisted\internet\posixbase.pyo", line 220, in run
 File "twisted\internet\posixbase.pyo", line 228, in mainLoop
--- <exception caught here> ---
 File "twisted\internet\base.pyo", line 561, in runUntilCurrent
 File "dtella_core.pyo", line 4198, in cb
 File "dtella.py", line 192, in startConnecting
 File "dtella_dnslookup.pyo", line 122, in getConfigFromDNS
 File "twisted\names\common.pyo", line 24, in query
 File "twisted\names\common.pyo", line 131, in lookupText
 File "twisted\names\client.pyo", line 313, in _lookup
 File "twisted\names\client.pyo", line 229, in queryUDP
 File "twisted\names\dns.pyo", line 1171, in query
 File "twisted\names\dns.pyo", line 1100, in writeMessage
 File "twisted\internet\udp.pyo", line 156, in write
socket.error: (10065, 'No route to host')

Attachments (1)

dns-query-except.patch (2.1 KB) - added by p1mrx 10 years ago.
Proposed patch

Download all attachments as: .zip

Change History (15)

comment:1 Changed 10 years ago by p1mrx

I already mentioned this on the mailing list, but a consequence of this bug is that, if writeMessage raises an exception, the value of 'resultDeferred' will be forgotten, causing a timeout to fire in the middle of nowhere a couple seconds later. That definitely needs to be fixed.

Changed 10 years ago by p1mrx

Attachment: dns-query-except.patch added

Proposed patch

comment:2 Changed 10 years ago by therve

Owner: changed from Jean-Paul Calderone to therve

comment:3 Changed 10 years ago by therve

(In [21264]) Apply patch, add tests.

Refs #2492

comment:4 Changed 10 years ago by therve

Description: modified (diff)

comment:5 Changed 10 years ago by therve

Cc: therve added
Keywords: review added
Owner: therve deleted
Priority: normalhighest

Ready to review in query-write-errors-2492.

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

Keywords: review removed
Owner: set to therve

I'd change the test_writeError docstring to say something like this:

Exceptions raised by the transport's write method should be turned into Failures passed to errbacks of the Deferred returned by L{<whichever protocol>.query}.

_query's docstring should talk about the errors which the returned Deferred can have, I guess.

The rest looks good.

comment:7 Changed 10 years ago by therve

(In [21378]) Some docs

Refs #2492

comment:8 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

Thanks, I added the docs above, back to review.

comment:9 Changed 10 years ago by Jonathan Lange

Keywords: review removed
Owner: set to therve

There is a substantial amount of fixture set up in the two new test_writeErrors. That set up makes it hard to see which parts actually exercise the code.

I think the best way to make this clearer is to extract two methods. The first method should create and return a protocol that uses the fake clock, the second should create and return a transport that raises an error on write.

Other than that, code looks good.

comment:10 Changed 10 years ago by therve

(In [21398]) Add setUp methods to clean the tests.

Refs #2492

comment:11 Changed 10 years ago by therve

Cc: Jonathan Lange added
Keywords: review added
Owner: therve deleted

I think I understood you concerns, but I tried to solve it using setUp, please tell me if it looks better. One of the good thing of this solution is that all the tests now use a deterministic clock, even if they don't explicitly manipulate it.

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

Keywords: review removed
Owner: set to therve

Looks good to me. Nice simplifications to the existing tests.

comment:13 Changed 9 years ago by therve

Resolution: fixed
Status: newclosed

(In [21535]) Merge query-write-errors-2492

Author: therve, p1mrx Reviewer: exarkun Fixes #2492

Handle write errors happening in dns queries, to have correct deferred failures.

comment:14 Changed 6 years ago by <automation>

Owner: therve deleted
Note: See TracTickets for help on using tickets.