Opened 2 years ago

Closed 13 months ago

#6095 enhancement closed fixed (fixed)

Get rid of the circular dependency between root and client in twisted.names

Reported by: exarkun Owned by: rwall
Priority: normal Milestone:
Component: names Keywords:
Cc: borko84@… Branch: branches/root-resolver-argument-6095-2
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

twisted.names.client uses twisted.names.root to construct the object returned by createResolver (sometimes). twisted.names.root uses twisted.names.client to bootstrap itself.

This circularity could be removed by giving twisted.names.root.Resolver a resolver argument so it doesn't need to import twisted.names.client to create one.

Attachments (1)

6095_v1.diff (3.0 KB) - added by borko 2 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 2 years ago by borko

  • Keywords review added

I guess it's obsolate. root imports only dns, common, error from twisted.names. Cannot see client. So I think it should be closed.

Changed 2 years ago by borko

comment:3 Changed 2 years ago by borko

  • Keywords review added

pls give an idea of easiest unit testing this one.

comment:4 Changed 2 years ago by borko

  • Author set to borko
  • Cc borko84@… added

comment:5 Changed 2 years ago by exarkun

  • Author borko deleted
  • Keywords review removed
  • Owner set to borko

As long as all the code is exercised by existing unit tests, this kind of change doesn't require new unit tests (perhaps partly because it is too difficult to test, and we lack the tools to do so, but also because the unit tests exist to demonstrate the code works, not that it has a particular shape - any shape that makes the unit tests pass is fine, as far as the tests go; and then as developers we can apply additional constraints like "shorter is better than longer" or "easier to read is better than harder to read").

You can use coverage.py to verify that the changed code really is exercised by the unit tests (or discover that it is not).

comment:6 Changed 17 months ago by rwall

  • Author set to rwall
  • Branch set to branches/root-resolver-argument-6095

(In [39243]) Branching to 'root-resolver-argument-6095'

comment:7 Changed 17 months ago by rwall

(In [39244]) Apply 6095_v1.diff from borko. Refs #6095

comment:8 Changed 17 months ago by rwall

(In [39320]) prototype FakeResolver for testing root.Resolver(resolver) and other dns client code. refs #6095, #6379

comment:9 Changed 17 months ago by rwall

  • Keywords review added
  • Owner borko deleted

Ready for review in log:branches/root-resolver-argument-6095

  1. I reviewed Borko's 6095_v1.diff which added a new positional "resolver" instance argument to root.Resolver. There are a few problems I see with that approach:
    1. root.bootstrap is public I don't think we can add a new mandatory positional argument. It has to be optional.
    2. I think its more flexible to be able to provide a factory function which is called by root.Resolver._query to build a client.Resolver (or some other Resolver instance)
    3. Borko, please comment if you have any counter arguments.
  1. So I went ahead and implemented it that way.
    1. Also added some tests for root.bootstrap (there weren't any before)
    2. Build Results
      1. http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/root-resolver-argument-6095

comment:10 Changed 14 months ago by tom.prince

I wonder if it would be appropriate to have an actual interface to represent the factory, rather than just a callable. One thing that confused me when I first started looking is what the servers argument.

Also, the interface the factory needs to return isn't IResolver, since it needs queryUDP (which is currently only defined on client.Resolver).

comment:11 Changed 13 months ago by exarkun

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

comment:12 follow-up: Changed 13 months ago by exarkun

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

Thanks! Sorry about the long review turn-around.

  1. twisted/names/root.py
    1. Let's think about deprecating some stuff here (separate tickets, of course). To completely remove the circular dependency we need to stop providing a default value for the resolverFactory parameter. However, bootstrap itself might be worth deprecating and replacing. Mainly because of the DeferredResolver return type. This should really be a Deferred that fires with an IResolver instead.
    2. I like Tom's idea of having an explicit interface for resolverFactory. I see no problem with doing this as a follow-up ticket.
    3. Thanks for the doc cleanups here!
    4. L{callable} is usually not what's meant though - I'd just write callable
  2. twisted/names/test/test_rootresolve.py
    1. The pass in the body of ResolverFactoryArguments is superfluous.
    2. In test_resolverFactoryOnlyExpectedArguments there's an assertion that depends on the order of the elements in the result of dict.keys. This should be made order-independent (set and sorted are handy - but maybe the assertion should just be against the dictionary directly).
    3. Strictly speaking I think that the reactor used in test_resolverFactorySuppliedReactor is a dummy rather than a stub. Those are both specific kinds of fake in case you don't want to bother being so precise. :) A dummy is an object that doesn't do anything. A stub is a minimal (usually no-op) implementation of an interface.
    4. I like having separate test cases for different units being tested - like BootstrapTests - because it makes naming easier. I think test_bootstrapReturnsDeferredResolver is redundant because it's part of BootstrapTests. test_returnsDeferredResolver is just as unambiguous and less of a mouthful. :)
    5. In test_bootstrapContinuesWhenAllRootHintsFail you might want to use TestCase.flushLoggedErrors instead of the addErrback trick.
    6. There's a minor whitespace issue or two, eg ((s,),{}).
  3. There's supposedly a new twistedchecker warning but it looks spurious to me.
  4. #6622 isn't exactly a duplicate of this ticket but it seems like it will be significantly affected by this work. If so it's probably worth leaving a comment there about what direction now makes sense.
  5. Some parts of Twisted Names have been ported to Python 3. Not this part, apparently. Think about whether the L{str} in the documentation should be L{bytes}, though, or whether that should wait until someone actually ports this module.

Thanks! Please merge after addressing these issues.

comment:13 Changed 13 months ago by rwall

  • Branch changed from branches/root-resolver-argument-6095 to branches/root-resolver-argument-6095-2

(In [40663]) Branching to 'root-resolver-argument-6095-2'

comment:14 in reply to: ↑ 12 Changed 13 months ago by rwall

  • Status changed from new to assigned

Replying to exarkun:

Thanks! Sorry about the long review turn-around.

  1. twisted/names/root.py
    1. Let's think about deprecating some stuff here (separate tickets, of course). To completely remove the circular dependency we need to stop providing a default value for the resolverFactory parameter. However, bootstrap itself might be worth deprecating and replacing. Mainly because of the DeferredResolver return type. This should really be a Deferred that fires with an IResolver instead.

#6822

  1. I like Tom's idea of having an explicit interface for resolverFactory. I see no problem with doing this as a follow-up ticket.

#6823

  1. Thanks for the doc cleanups here!
  2. L{callable} is usually not what's meant though - I'd just write callable

r40657

  1. twisted/names/test/test_rootresolve.py
    1. The pass in the body of ResolverFactoryArguments is superfluous.

r40658

  1. In test_resolverFactoryOnlyExpectedArguments there's an assertion that depends on the order of the elements in the result of dict.keys. This should be made order-independent (set and sorted are handy - but maybe the assertion should just be against the dictionary directly).
  2. Strictly speaking I think that the reactor used in test_resolverFactorySuppliedReactor is a dummy rather than a stub. Those are both specific kinds of fake in case you don't want to bother being so precise. :) A dummy is an object that doesn't do anything. A stub is a minimal (usually no-op) implementation of an interface.

r40659

  1. I like having separate test cases for different units being tested - like BootstrapTests - because it makes naming easier. I think test_bootstrapReturnsDeferredResolver is redundant because it's part of BootstrapTests. test_returnsDeferredResolver is just as unambiguous and less of a mouthful. :)

r40660

  1. In test_bootstrapContinuesWhenAllRootHintsFail you might want to use TestCase.flushLoggedErrors instead of the addErrback trick.

r40662

  1. There's a minor whitespace issue or two, eg ((s,),{}).

r40661.

  1. There's supposedly a new twistedchecker warning but it looks spurious to me.

Yep. Spurious. My branches always get that error. Not sure why?

  1. #6622 isn't exactly a duplicate of this ticket but it seems like it will be significantly affected by this work. If so it's probably worth leaving a comment there about what direction now makes sense.

ticket:6622#comment:13

  1. Some parts of Twisted Names have been ported to Python 3. Not this part, apparently. Think about whether the L{str} in the documentation should be L{bytes}, though, or whether that should wait until someone actually ports this module.

I decided not do that here. Too tired and the Python3 builder is offline anyway.

Thanks again for the review.

I'll merge as soon as the builders are back online and I get some clean builds.

comment:15 Changed 13 months ago by rwall

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [40770]) Merge root-resolver-argument-6095-2: Remove circular dependency between root and client

Authors: borko, rwall
Reviewers: tom.prince, exarkun
Fixes: #6095
Refs: #6622, #6822, #6823

Partially removed the circular dependency between twisted.names.root and
twisted.names.client. #6822 will be the next step in completely removing the
dependency.

In doing this, twisted.names.root.Resolver now accepts a resolverFactory
argument, which makes it possible to control how root.Resolver performs
iterative queries to authoritative nameservers.

Also added test coverage for twisted.names.root.bootstrap.

Also added and updated various API documentation in root.

Note: See TracTickets for help on using tickets.