9032 HostnameEndpoint Memory Reactor #740

markrwilliams · 2017-03-11T08:50:54Z

http://twistedmatrix.com/trac/ticket/9032

This aims to fix a release blocker.

…reactor adapt the reactor instead of the application

…Endpoint. Add deprecation warning. Fix a bug in _SimpleHostnameResolver._deliver. Tests for _SimpleHostnameResolver and the revived HostnameEndpoint behavior.

This reduces the overall diff against trunk and eliminates an opportunity for twistedchecker to complain about missing docstrings.

These tests mostly fail.

The refactoring allows the deprecation warning to sit in _StandardEndpoint.__init__, which is one stack frame closer to Agent.__init__. It also allows for docstrings that link to a comment in 9032.

codecov · 2017-03-11T09:04:18Z

Codecov Report

Merging #740 into trunk will increase coverage by 0.01%.
The diff coverage is 99.42%.

@@            Coverage Diff             @@
##            trunk     #740      +/-   ##
==========================================
+ Coverage   89.99%      90%   +0.01%     
==========================================
  Files         841      841              
  Lines      146544   146710     +166     
  Branches    12848    12856       +8     
==========================================
+ Hits       131884   132049     +165     
  Misses      12291    12291              
- Partials     2369     2370       +1

markrwilliams · 2017-03-11T23:05:11Z

There's an unrelated OpenSSL issue that's causing the Windows and macOS builders to fail.

EDIT #743 is an attempt at fixing those.

This reverts commit 748f05f.

This reverts commit a55f7b9.

This reverts commit accd508.

tomprince

This looks like a good improvement. There are a couple of small changes, and I didn't have time to look at the tests.

tomprince · 2017-03-31T03:41:11Z

src/twisted/internet/endpoints.py

+                reactor.__class__,
+                Version("Twisted", "NEXT", 0, 0),
+                format=("Passing HostnameEndpoint %(fqpn)s"
+                        " was deprecated in %(version)s"),


It isn't clear to me that this should be calling out passing a specific type as being deprecated. I think it would be better to simply say passing a reactor that doesn't implement IReactorPluggableNameResolver is deprecated. Particularly since the fix might be to make the reactor implement the new interface, rather than changing the reactor being passed.

The log message should include the interface required, but I'd like it to also include the FQPN of the reactor to make debugging tests easier.

Including the FQPN of the reactor seems fine. But I'd go with something like:

Passing a reactor that doesn't implement IReactorPluggableNameResolver to HostnameEndpoint was deprecated <...>. (%(fqpn)s was passed, which doesn't currently implement <...>

that makes it clear that a possible fix is making the reactor implement the given interface.

I guess it already does mention the interface required of the reactor.

tomprince · 2017-03-31T03:48:46Z

src/twisted/web/test/test_agent.py

@@ -1425,7 +1425,7 @@ def test_deprecatedDuckPolicy(self):
        WebClientContextFactory} to do it.
        """
        def warnMe():
-            client.Agent(MemoryReactorClock(),
+            client.Agent(deterministicResolvingReactor(MemoryReactorClock()),


Can you file a ticket for either making this public, or extending MemoryReactorClock with this functionality?

Done! https://twistedmatrix.com/trac/ticket/9103

twisted.testing HERE WE COME

tomprince · 2017-03-31T03:52:14Z

src/twisted/topfiles/9032.bugfix

@@ -0,0 +1 @@
+twisted.internet.endpoints.HostnameEndpoint and twisted.web.client.Agent work again with reactors that do not provide IReactorPluggableNameResolver.  This undoes the changes that broke downstream users such as treq.testing.  Note that passing reactors that do not provide IReactorPluggableNameResolver to either is deprecated.


I think this would be better split into two news files. One for the bugfix, and another one for the deprecation. Also, I don't think it makes sense to call out that this is undoing a previous change: the new behavior is actually different than the old and the point isn't to undo it, but just provide proper deprecation warnings for the change.

tomprince · 2017-03-31T03:54:33Z

src/twisted/internet/endpoints.py

+
+    @ivar _nameResolution: the callable L{resolveHostName} invokes to
+        resolve hostnames.
+    @type _nameResolution: A L{callable} with the same signature as


There are some missing words here.

tomprince · 2017-03-31T04:04:02Z

src/twisted/internet/endpoints.py

+        resolutionReceiver.resolutionBegan(HostResolution(hostName))
+        d = self._nameResolution(hostName, portNumber)
+        d.addCallback(self._deliver, resolutionReceiver)
+        d.addErrback(self._ebLog, hostName)


I don't think there is any value in having these callbacks out-of-line. There is a bunch of code like this in twisted, but I think that newer code tends to avoid it.

It does - I took this approach because I had pretensions of testing each of these methods. But coverage is good, so I might as well switch back to closures.

Also the docstrings looked nicer on instance methods than they did on closures.

If they are closures, they might not need docstrings (certainly they could do with more abbreviated docstrings in that case).

They don't - I found them useful while writing the patch, so I thought I'd leave them in.

tomprince · 2017-03-31T04:08:22Z

src/twisted/internet/endpoints.py

+    def _nameResolution(self, host, port):
+        """
+        Resolve the hostname string into a tuple containing the host
+        address.


It would probably be good to make it clear at least in the docstring and perhaps in the method name that this method is just used as a fallback when the reactor provided to the endpoint doesn't implement the new interface.

tomprince · 2017-03-31T04:09:40Z

src/twisted/internet/endpoints.py

+            return value.
+        """
+        return self._deferToThread(self._getaddrinfo, host, port, 0,
+                socket.SOCK_STREAM)


I'm not sure that it makes sense to allow overriding deferToThread and getaddrinfo individually, instead of just overriding this function.

deferToThread has to be replaceable to make this function testable. The original implementation also made getaddrinfo replaceable, apparently with the intent of verifying that deferToThread invokes it.

I was inclined to leave these two staticmethods in for two reasons:

My tests looked more like the existing ones, apparently verifying the same behaviors.

A vague concern that some tests, inside or outside Twisted itself, relied on it.

A cursory GitHub search indicated that 2. wasn't the case. 1. remains true and useful, I think.

Looking at the tests, everywhere that ._deferToThread and ._getaddrinfo are overridden could be replaced by

self._nameResolution = fakegetaddrinfo

(modulo fakegetaddrinfo taking some additional arguments that it ignores). That would mean that _nameResolution isn't covered by the tests but I don't think the coverage that it currently gets is meaningful, since in tests it is just calling fake function that aren't verified to be equivalent to the real thing anyway.

@tomprince I've filed a new ticket that we can use to track this improvement.

tomprince · 2017-03-31T04:14:07Z

src/twisted/internet/test/test_endpoints.py

+
+    def connectArgs(self):
+        """
+


There is an extra newline here.

twistedchecker complains if there isn't a newline here, and it complains if the return type isn't documented.

That seems like a bug in twistedchecker. Or an indication there should be some prose here.

I think it's a bug in twistedchecker. All of the other test classes that define connectArgs have the same docstring, minus the newline, presumably to quiet twistedchecker.

Please file & link the bug in twistedchecker so we know about it, per policy :)

Looks like @rodrigc filed the bug: twisted/twistedchecker#124

See #740 (review)

Windows leaves this as AF_UNSPEC if you don't specify it, and we shouldn't test against it anyway in case quic or something makes it into a kernel.

glyph

Some minor observations inline, but pending a clean CI run I think this is more than baked. Please address and land.

glyph · 2017-05-23T18:48:45Z

src/twisted/internet/test/test_endpoints.py

+
+class HostnameEndpointFallbackNameResolutionTests(unittest.TestCase):
+    """
+    Verify that L{HostnameEndpoint._fallbackNameResolution} defers a


"verify that" -> https://jml.io/pages/test-docstrings.html

glyph · 2017-05-23T18:49:15Z

src/twisted/internet/test/test_endpoints.py

+
+    def test_fallbackNameResolution(self):
+        """
+        A L{Deferred} is returned that fires with the resoution of the


Active voice not passive voice, "_fallbackNameResolution returns a Deferred that..."

glyph · 2017-05-23T18:49:39Z

src/twisted/internet/test/test_endpoints.py

+
+    def connectArgs(self):
+        """
+


Please file & link the bug in twistedchecker so we know about it, per policy :)

glyph · 2017-05-23T18:49:54Z

src/twisted/internet/test/test_endpoints.py

+
+    def test_endpointConnectFailure(self):
+        """
+        If L{HostnameEndpoint.connect} is invoked and there is no server


"when" is preferable to "if", because it stipulates the existence of a supported configuration.

glyph · 2017-05-23T18:50:44Z

src/twisted/web/test/test_agent.py

@@ -1425,7 +1425,7 @@ def test_deprecatedDuckPolicy(self):
        WebClientContextFactory} to do it.
        """
        def warnMe():
-            client.Agent(MemoryReactorClock(),
+            client.Agent(deterministicResolvingReactor(MemoryReactorClock()),


twisted.testing HERE WE COME

(see #740 (review))

markrwilliams · 2017-05-23T21:41:24Z

The failing Codecov status appears to be down to partial coverage on this line. That could be covered by resolving an IPv6 address.

We now skip tests that need IPv6 on platforms that don't support it, but the way we determine that needs to be generalized. Rather than introduce another instance of this function's definition in this patch, I'll merge with the slightly reduced coverage and address it in the patch for 9146.

markrwilliams and others added 14 commits February 24, 2017 16:51

Ressurect old HostnameEndpoint connect method.

4c64334

Existing HostnameEndpoint should implement, not provide.

ff71aa0

Make treq tests pass by avoiding HostnameEndpoint.

accd508

adapt the reactor instead of the application

c0ad3fc

Merge pull request #718 from exarkun/9032-hostnameendpoint-old-memory…

098ada9

…reactor adapt the reactor instead of the application

Use twisted.logger instead of twisted.python.log

ae6b01f

Wrap a long line.

8b8e04b

Remove _PluggableReactorAdapter by lifting nameResolver onto Hostname…

9b5d4da

…Endpoint. Add deprecation warning. Fix a bug in _SimpleHostnameResolver._deliver. Tests for _SimpleHostnameResolver and the revived HostnameEndpoint behavior.

Fold HostnameEndpoint._connectWithNameResolver into .connect.

6d56832

This reduces the overall diff against trunk and eliminates an opportunity for twistedchecker to complain about missing docstrings.

Add tests for t.w.client._StandardEndpoint and MemoryReactors.

748f05f

These tests mostly fail.

Refactor t.w.client._StandardEndpoint fallbacks and deprecate.

a55f7b9

The refactoring allows the deprecation warning to sit in _StandardEndpoint.__init__, which is one stack frame closer to Agent.__init__. It also allows for docstrings that link to a comment in 9032.

Topfile for 9032

6a64176

Merge branch 'trunk' into 9032-hostnameendpoint-old-memoryreactor

78fe173

Methods and URLs are bytes; call Agent.request before deprecation check

a631ea1

markrwilliams added 3 commits March 11, 2017 01:08

URLs are actually bytes.

2db0324

Avoid new deprecation warning by using a compliant reactor.

fbd7eae

Fix method reference.

c03edff

markrwilliams and others added 8 commits March 20, 2017 07:29

Merge branch 'trunk' into 9032-hostnameendpoint-old-memoryreactor

4154ba3

Revert "Add tests for t.w.client._StandardEndpoint and MemoryReactors."

023fcad

This reverts commit 748f05f.

Revert "Refactor t.w.client._StandardEndpoint fallbacks and deprecate."

3485740

This reverts commit a55f7b9.

Revert "Make treq tests pass by avoiding HostnameEndpoint."

1c9171f

This reverts commit accd508.

Merge branch 'trunk' into 9032-hostnameendpoint-old-memoryreactor

4b4d8e7

Merge branch 'trunk' into 9032-hostnameendpoint-old-memoryreactor

443eb69

Merge branch 'trunk' into 9032-hostnameendpoint-old-memoryreactor

f5befe4

Merge branch 'trunk' into 9032-hostnameendpoint-old-memoryreactor

f70740e

tomprince suggested changes Mar 31, 2017

View reviewed changes

markrwilliams and others added 2 commits April 3, 2017 08:30

Merge branch 'trunk' into 9032-hostnameendpoint-old-memoryreactor

1f27076

Address review feedback.

0135661

See #740 (review)

markrwilliams added 5 commits April 3, 2017 17:16

Test _fallbackNameResolution explicitly.

dfa51e8

IP addresses are text on Python 3.

abe1197

Don't check the protocol in getaddrinfo.

4c7ca0c

Windows leaves this as AF_UNSPEC if you don't specify it, and we shouldn't test against it anyway in case quic or something makes it into a kernel.

Merge branch 'trunk' into 9032-hostnameendpoint-old-memoryreactor

9c985fc

topfile -> newsfragments

dab4a94

glyph approved these changes May 23, 2017

View reviewed changes

Improve docstrings per @glyph's review. Thanks!

4cc9880

(see #740 (review))

markrwilliams merged commit b278d2e into trunk May 23, 2017

markrwilliams deleted the 9032-hostnameendpoint-old-memoryreactor branch May 23, 2017 21:45

		@@ -0,0 +1 @@
		twisted.internet.endpoints.HostnameEndpoint and twisted.web.client.Agent work again with reactors that do not provide IReactorPluggableNameResolver. This undoes the changes that broke downstream users such as treq.testing. Note that passing reactors that do not provide IReactorPluggableNameResolver to either is deprecated.

9032 HostnameEndpoint Memory Reactor #740

9032 HostnameEndpoint Memory Reactor #740

Conversation

markrwilliams commented Mar 11, 2017

codecov bot commented Mar 11, 2017 • edited Loading

Codecov Report

markrwilliams commented Mar 11, 2017 • edited Loading

tomprince left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markrwilliams commented May 23, 2017

codecov bot commented Mar 11, 2017 •

edited

Loading

markrwilliams commented Mar 11, 2017 •

edited

Loading