Opened 13 years ago

Last modified 7 years ago

#2336 task new

timeouts in twisted.names.client are poorly documented and respected.

Reported by: Stephen Thorne Owned by: Mark
Priority: normal Milestone:
Component: names Keywords:
Cc: Mark, Thijs Triemstra Branch:
Author:

Description

See twisted.names.client.Resolver.lookupZone.

The entire issue of timeouts seems schitophrenic. IResolver interface says that the default timeout is '10', while None or a sequence of ints is expected in most instances.

lookupZone takes an int timeout, while lookupAnythingElse taks a sequence of int.

Please clean up twisted.internet.interfaces.IResolver and twisted.names.client to be consistently documented and implemented.

Change History (4)

comment:1 Changed 9 years ago by Mark

Cc: Mark added
Owner: changed from Jean-Paul Calderone to Mark

comment:2 Changed 9 years ago by Mark

Notes:

  • implementors of IResolver don't have lookupRecord defined (maybe this should go into another ticket?)
  • twisted.names.common.ResolverBase IResolver.query implementation uses query(query, timeout = None) signature, while the interface lists this one: query(query, timeout = 10). I guess, that means, that the interface definition has to use None as default, as well
  • all lookup* methods, that correspond to actual RR types list 10 as the default timeout argument value, while all implementors use None and use a sequence of ints when not explicitly provided with a value
  • lookupZone uses the default value of 10, but that is consistent with the implementations, that also list 10, since its a TCP-only query

I am proposing the following changes:

  • for all lookup* methods, that correspond to actual RR types, change the default timeout argument value to None and change the wording of the docstring to something like this:
    def lookupAddress(name, timeout=None):
        """
        Lookup the A records associated with C{name}.
    
        @type name: C{str}
        @param name: DNS name to resolve.
    
        @type timeout: C{None} or sequence of C{int}
        @param timeout: Number of seconds after which to reissue the query.
        When the last timeout expires, the query is considered failed.
        """
    
  • change the default value for timeout in IResolver.query to None as well, since it just delegates to other lookup* methods. lookupZone will still work if None is passed, since it defaults to 10 internally (write a test for this?)
  • leave lookupZone signature as it is (timeout = 10), because it is consistent with the implementation

I will post an actual patch once #3928 is resolved, since it introduces a new RR type and, subsequently, a new method in IResolver definition (that also needs to be converted to the other signature).

comment:3 Changed 8 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

comment:4 Changed 7 years ago by Richard Wall

I implemented most of the suggested improvements in #4685.

See the patch and comment 3 on that ticket.

Note: See TracTickets for help on using tickets.