Opened 13 months ago

Last modified 11 months ago

#6644 enhancement new

twisted.names.dns.DNSMixin._query should return a cancellable Deferred.

Reported by: kaizhang Owned by: kaizhang
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch: branches/dnsmixin-deferred-cancellation-6644-2
(diff, github, buildbot, log)
Author: kaizhang Launchpad Bug:

Description

twisted.names.dns.DNSMixin._query should return a cancellable Deferred. When cancelling the Deferred, resultDeferred and cancelCall should be removed from DNSMixin.liveMessages. And the cancelCall should be cancelled.

Change History (15)

comment:1 Changed 13 months ago by kaizhang

  • Owner set to kaizhang
  • Status changed from new to assigned

comment:2 Changed 13 months ago by kaizhang

  • Author set to kaizhang
  • Branch set to branches/dnsmixin-deferred-cancellation-6644

(In [39315]) Branching to 'dnsmixin-deferred-cancellation-6644'

comment:3 Changed 13 months ago by kaizhang

  • Keywords review added
  • Owner kaizhang deleted
  • Status changed from assigned to new

comment:4 Changed 13 months ago by kaizhang

  • Keywords review removed
  • Owner set to kaizhang

Need to add news file.

comment:5 Changed 13 months ago by kaizhang

  • Keywords review added
  • Owner kaizhang deleted

comment:6 follow-up: Changed 13 months ago by itamar

You should have a test for the case where a response does eventually arrive after cancellation, making sure nothing breaks and it's just dropped with no exceptions.

comment:7 in reply to: ↑ 6 Changed 13 months ago by kaizhang

Replying to itamar:

You should have a test for the case where a response does eventually arrive after cancellation, making sure nothing breaks and it's just dropped with no exceptions.

I added the tests for this case in another ticket since the arrived response is handle by DNSProtocol/DNSDatagramProtocol. The ticket is #6655. When I wrote the tests I found a bug about twisted.names.test.test_dns.TestTCPController so I fixed the bug and added the tests in a new ticket.

comment:8 Changed 13 months ago by rwall

There are some questions in ticket:6659#comment:2 about whether the timeout parameter provided to queryTCP and lookupZone should be applied overall ie time taken to establish TCP connection and time taken for complete DNS response.

And if so how does it fit in with this new deferred cancellation function?

comment:9 Changed 13 months ago by itamar

I'll add comment about overall timeouts in #6659. As far as Deferred cancellation, it is a nice way to implement timeouts (untested, but there's a ticket somewhere to add this to Twisted):

def timeout(deferred, seconds):
    delayedCall = reactor.callLater(seconds, deferred.cancel)
    def checkCanceled(result):
        if delayedCall.active(): delayedCall.cancel()
        return result
    deferred.addBoth(checkCanceled)

comment:10 Changed 13 months ago by kaizhang

  • Keywords review removed
  • Owner set to kaizhang
  • Status changed from new to assigned

Need to rebranch to get the tests for response arriving after cancellation from #6655.

comment:11 Changed 13 months ago by kaizhang

  • Branch changed from branches/dnsmixin-deferred-cancellation-6644 to branches/dnsmixin-deferred-cancellation-6644-2

(In [39658]) Branching to 'dnsmixin-deferred-cancellation-6644-2'

comment:12 Changed 13 months ago by kaizhang

  • Keywords review added
  • Owner kaizhang deleted
  • Status changed from assigned to new

comment:13 Changed 11 months ago by tom.prince

A couple of comments:

  1. test_cancelQueryCancelTheCancelCall, you should cancel the deferred returned by _query rather than the one stored in liveMessages. They are the same, but the later is an implementation detail.
  2. If a request is cancelled, subsequent responses with that id are passed to the DNS controller. This is also what happens whenthe timeout is reached. I think it would make more sense to silently drop them. twisted.names.client.Resolver will log any messages that it receives (although, it ends up not being an issue in this case, since the protocol is closed immediately when the query is cancelled).

comment:14 Changed 11 months ago by rwall

  • Owner set to rwall
  • Status changed from new to assigned

comment:15 Changed 11 months ago by rwall

  • Keywords review removed
  • Owner changed from rwall to kaizhang
  • Status changed from assigned to new

Thanks kaizhang,

Sorry for the delay in actually reviewing this ticket.

Notes:

  • Merges cleanly
  • Tests pass locally
  • Coverage is complete
  • Builds all green (apart from a couple of spurious twistedchecker warnings)

In addition to Tom's numbered points above:

  1. twisted/names/dns.py
    1. I don't like the fact that the nested function refers to variable that is defined after the function. I know it works, but somehow it doesn't look right to me. I also not that you've used this technique in previous cancellation branches which have been accepted and merged.
    2. So consider making cancel a method instead of a nested function. eg
      Index: twisted/names/dns.py
      ===================================================================
      --- twisted/names/dns.py        (revision 40401)
      +++ twisted/names/dns.py        (working copy)
      @@ -2150,23 +2150,29 @@
      except:
      return defer.fail()
      
      - def cancel(deferred):
      - """
      - Cancel the query.
      -
      - Remove the C{resultDeferred} and C{cancelCall} from
      - L{self.liveMessages}. Cancel the C{cancelCall}.
      -
      - @param deferred: The cancelled L{defer.Deferred}.
      - """
      - del self.liveMessages[id]
      - cancelCall.cancel()
      - resultDeferred = defer.Deferred(cancel)
      - resultDeferred = defer.Deferred(self._queryCancel)
        cancelCall = self.callLater(timeout, self._clearFailed, resultDeferred, id)
        self.liveMessages[id] = (resultDeferred, cancelCall)
      
        return resultDeferred
      
      -
      - def _queryCancel(self, deferred):
      - """
      - Cancel the query.
      -
      - Remove the C{resultDeferred} and C{cancelCall} from
      - L{self.liveMessages}. Cancel the C{cancelCall}.
      -
      - @param deferred: The cancelled L{defer.Deferred}.
      - """
      - for id, (resultDeferred, cancelCall) in self.liveMessages.items():
      - if resultDeferred is deferred:
      - del self.liveMessages[id]
      - cancelCall.cancel()
      - break
      -
      -
                 def _clearFailed(self, deferred, id):
                     """
                     Clean the Deferred after a timeout.
      
    3. I also wonder if this is slightly safer. Imagine if someone called DNSDatagramProtocol.query with a duplicate ID, the original entry in liveMessages would be overwritten and a cancellation of the original query would then delete the wrong liveMessages entry.
    4. The *cancel* docstring contains *L{self.liveMessages}* which I don't think is valid. L{} has to refer to an object that can be found by PyDoctor.
    5. I've been thinking about itamar's proposed timeout function which calls cancel. It sounds neat, but what about if I need to distinguish between a cancellation and a timeout. A Timeout would probably result in a scheduled retry whereas cancellation would not. Is there anyway to customise the type of Failure in the errback as a result of cancellation?
    6. Should cancellation also close the underlying transport? I was considering what happens if a result eventually comes back from a cancelled query. It will be treated as a new message and dispatched to controller.messageReceived.
      1. Tom makes the same point in comment:13 -- he suggests silently dropping the message, which would require some extra logic in data and datagram received....I think (plus associated tests)
      2. I wonder if cancellation should just terminate the connection (in the case of TCP) or close the port (in the case of UDP)?
      3. ...but then again, what if the user wants to issue multiple queries using the same DNSProtocol instance? I can't think why, since the reason we've cancelled the query is presumably that the server that we're talking to has not responded, so why bother sending further queries or expecting results to other outstanding queries to that server.
  2. twisted/names/test/test_dns.py
    1. It doesn't seem right to be testing only the DNSMixin class.
      1. Wouldn't it be better to test the DNSDatagramProtocol and DNSProtocol instead (or in addition).
      2. I think that would make it easier to refactor the public DNS protocols in the future. (ie if we wanted to replace or remove the DNSMixin...which I'd quite like to do because it confuses me).
      3. In any case, I think it might be clearer to call the test case CancellationTests or CancellationTestCase.
      4. Say "is" instead of "should be" in the new test docstrings.
  1. twisted/names/topfiles/6644.feature
    1. The news file should refer to the affected public APIs, not DNSMixin._query.

Here are some situations where I think query cancellation will be useful.

  • client.Resolver -- cancelling an unanswered UDP query before issuing a TCP query.
  • EDNSProtocol -- cancelling an unanswered EDNS query before issuing another query with a smaller UDP packet size.
  • root.Resolver -- issuing parallel queries to all authoritative nameservers and cancelling outstanding queries after the first response is received.
  • DNSSEC verification -- again, I imagine we will issue parallel queries to auth servers and lock onto the server which responds fastest, cancelling other outstanding queries.
  • GAI endpoint -- where IPv4 and IPv6 UDP DNS queries are issued in parallel, cancel the oustanding query when the first response is received.

Anyway, thanks again. Please address or answer the numbered points above
(including Tom's) and resubmit for another review.

-RichardW.

Note: See TracTickets for help on using tickets.