Close the HTTP pool when service is stopped. Take 2. #156

glyph · 2020-02-24T04:56:50Z

Scope

This tries to fixes #86

When the service is stopped, it should automatically clean the resource and not left files opened or connections openend.

In Take1 the #154 regression was introduced.

Obviously there was lots of great stuff in code I had to revert out, so we should probably take a second crack at this.

This is the take 2 of #140

…ed.""

glyph · 2020-02-24T04:57:45Z

(I would suggest splitting this up into a bunch of smaller PRs, the PR template and contributing docs don't seem to have anything to do with the functional changes here.)

codecov · 2020-02-24T05:18:26Z

Codecov Report

Merging #156 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            master      #156    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           27        27            
  Lines         2000      2100   +100     
  Branches       173       182     +9     
==========================================
+ Hits          2000      2100   +100

Impacted Files	Coverage Δ
src/txacme/client.py	`100.00% <100.00%> (ø)`
src/txacme/endpoint.py	`100.00% <100.00%> (ø)`
src/txacme/service.py	`100.00% <100.00%> (ø)`
src/txacme/test/strategies.py	`100.00% <100.00%> (ø)`
src/txacme/test/test_client.py	`100.00% <100.00%> (ø)`
src/txacme/test/test_endpoint.py	`100.00% <100.00%> (ø)`
src/txacme/test/test_service.py	`100.00% <100.00%> (ø)`
src/txacme/testing.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1016e02...199858a. Read the comment docs.

adiroiban · 2020-02-28T07:38:01Z

Thanks for the work on this.

I split it into a single other PR ... see here #157

I will look and try to fix the defect

adiroiban · 2020-02-28T07:46:31Z

@glyph this was a backward incompatible change.

As long as the fix for #150 is not merged and released I will leave this change unmerged,

I will add this change in my acme-v2 branch.... as that branch breaks the whole API.

glyph · 2020-02-28T17:24:44Z

@glyph this was a backward incompatible change.

I'll have to address backwards incompatibility when I update lancer to deal with more recent versions; that's fine. But that's not what I saw here: I didn't revert it because it was backwards incompatible; I reverted it because it was incompatible with itself (the still-included endpoint, which I'm still using in production despite the fact that the SNI challenge doesn't work, raised exceptions and refused to listen on the port).

As long as the fix for #150 is not merged and released I will leave this change unmerged,

#150 is merged, I should do a release.

I will add this change in my acme-v2 branch.... as that branch breaks the whole API.

Again, the incompatibilities are not a huge concern here; I'd much rather land this as a separate change if we can, just to make the review churn manageable. (Although perhaps you are correct that we should do a release with #150 and without this, to provide a smoother upgrade path.)

Thank you for working on the acme-v2 branch though! Is it already working for you, or just in progress?

glyph · 2020-02-28T17:25:17Z

it was incompatible with itself

incompatible with itself and also the docs were never updated so they were still wrong :)

adiroiban · 2020-02-28T19:07:32Z

@glyph thanks for the info.

I tried to update the docstring for AutoTLSEndpoint.

Can you please take a look and let me know if you find any other errors?

Thanks!

The acme v2 works for me ... and if you are just using the AcmeIssuingService it should be backward compatible with anyone.

txacme.client.Client.fromURL API is not changed....

But I have added txacme.client.Client.start() to match the the new stop() method from here...and starts also takes care of registration and getting a first nonce.

The SAN is implemented using CSV names.

adiroiban · 2020-03-13T08:33:10Z

@glyph do you have time to check that the changes are ok now?

I want to have this merges as it can help with the future tests that I want to add for ACME v2 support.

Thanks!

glyph · 2020-03-13T21:41:09Z

@adiroiban I'll try to check it out this weekend; it would be great if you could address whatever's wrong on Travis before then, though!

adiroiban · 2020-03-14T06:47:56Z

I have retried the failings jobs... no is green (sort of :)

It looks like one failure is due to a flaky test while the other was a pip install error.

hypothesis.errors.Flaky: Hypothesis test_poll_timeout(self=<txacme.test.test_client.ClientTests.test_poll_timeout id=0x80de720>, name='daaa') produces unreliable results: Falsified on the first call but did not on a subsequent one

Thanks!

glyph · 2020-03-15T06:52:05Z

@adiroiban I know it's a bit of an artificial metric, but it seems a shame to sacrifice 100% code coverage. Would you mind fixing up the coverage around stop and tearDown (in the latter case, just doing a coverage exclusion would be OK by me; it is test code after all) https://codecov.io/gh/twisted/txacme/compare/4288f5d876bb46b160db72406732c50875388eed...a474bd4c6bbc1561e4303a413aa2cd66d9b8fd99/diff#D3-814

glyph

Apparently I can't approve this PR because it's "mine" anyway, but I wanted to ask for getting the test coverage back to 100% anyway. :). A couple of other minor issues inline, but if you can address these i'll merge whether I can 'approve' or not.

glyph · 2020-03-15T06:54:07Z

src/txacme/endpoint.py

@@ -84,7 +81,7 @@ class AutoTLSEndpoint(object):
    reactor = attr.ib()
    directory = attr.ib(
        validator=lambda inst, a, value: check_directory_url_type(value))
-    client_creator = attr.ib()
+    client = attr.ib()


Would you consider putting a validator in here, to make it easier for folks to find the place where they need to update their code? In the absence of proper type checking, it would be easy for anyone invoking this to pass the wrong thing, and the error wouldn't be reported until later. I know we don't have a strict Twisted-style compat policy here, but it would still be nice.

src/txacme/newsfragments/86.bugfix

glyph · 2020-03-15T06:55:36Z

src/txacme/newsfragments/86.bugfix

@@ -0,0 +1,4 @@
+INCOMPATIBLE CHANGE: txacme.service.AcmeIssuingService.stopFactory nows
+closes the persisted HTTP client connections.
+This is done to bring the in a state similar to the one before calling


What does "bring the in" mean here?

I think that the last sentence can be removed.

src/txacme/newsfragments/86.removal

src/txacme/test/test_client.py

src/txacme/test/test_endpoint.py

glyph · 2020-03-15T06:59:53Z

src/txacme/testing.py

@@ -19,6 +21,24 @@
 from txacme.util import clock_now, generate_private_key


+class TXACMETestCase(TestCase):


Could you change this from a superclass into a function?

Rather than inheriting a tearDown, which can be quite challenging and difficult to correctly inherit and compose, you could make it work like this:

def cancelDelayedCallsAtExit(testCase): def cancelCalls(): ... testCase.addCleanup(cancelCalls)

This avoids having a big ball of random utility functionality accrete around all the tests.

In fact, I think you can just use AsynchronousDeferredRunTest; add run_tests_with = AsynchronousDeferredRunTest to the test cases needing this.

See https://testtools.readthedocs.io/en/latest/twisted-support.html#running-tests-in-the-reactor and https://github.com/testing-cabal/testtools/blob/209c1bf7618541f7b9a9e01798ef0588658d6ab1/testtools/twistedsupport/_runtest.py#L452

Happy to use a function... but the idea is that we want all tests to be executed with this check..and to make sure you don't forget to enable this check.

I agree that setUp / tearDown inheritance can be complicated, but at the same time I think that we should enforce that this check is called for all tests.

If we make the check optional, people might forget to enable it for the tests

It's equally possible to forget to inherit from the test class though; like it's literally the same number of modified lines of code. (one per class statement or one per setUp method).

However, there are other extension mechanisms we could use for this: the test_suite hook for example?

glyph · 2020-03-15T07:00:17Z

src/txacme/testing.py

@@ -19,6 +21,24 @@
 from txacme.util import clock_now, generate_private_key


+class TXACMETestCase(TestCase):
+    """
+    Common code for all tests for the txacme project.


I definitely don't want all the tests in the whole project to have special nonstandard behavior :)

mithrandi · 2020-03-15T07:30:21Z

src/txacme/test/test_client.py

@@ -344,6 +344,7 @@ def test_unexpected_update(self):
            self.assertThat(
                client.register(reg2),
                failed_with(IsInstance(errors.UnexpectedUpdate)))
+        succeeded(client.stop())


This constructs a matcher then does nothing with it; I think you want an assertThat call here.

True. Thanks.

glyph · 2020-08-17T23:09:54Z

@adiroiban any chance you'll have some time to work on this soon?

adiroiban · 2020-08-18T00:42:16Z

I will not have time to work on this anytime soon. Feel free to continue the work.

I am using the code from this branch https://github.com/twisted/txacme/tree/151-acme-v2 and it works for me.

and hey it looks like stop() didn't actually work right due to the .called check

glyph · 2020-08-26T10:25:48Z

I am going to just land this because we need to get v2 out for this to be useful at all, now that the LE endpoints have shut down new registrations and are starting brownouts.

adiroiban · 2020-08-26T13:11:50Z

I am sorry that I was not able to create a proper pull request.
But I think that share libraries should be as lightweight as possible.
For me eliot and hypothesis were a blocker as I am not familiar with those libraries and I didn't had time to learn how to use them.

I had the same issue that v2 should be delivered ASAP, so I went ahead and implemented txacme with v2 without eliot and with stdlib unit tests.

Revert "Revert "[Fixes #86] Close the HTTP pool when service is stopp…

02fc4cc

…ed.""

adiroiban changed the title ~~Revert "Revert "[Fixes #86] Close the HTTP pool when service is stopped.""~~ Close the HTTP pool when service is stopped. Take 2. Feb 28, 2020

adiroiban closed this Feb 28, 2020

adiroiban reopened this Feb 28, 2020

adiroiban added 2 commits February 28, 2020 19:00

Merge master.

3e75843

Update the endpoint docstring.

a474bd4

adiroiban mentioned this pull request Feb 28, 2020

Initial support for ACME v2 #158

Closed

7 tasks

glyph commented Mar 15, 2020

View reviewed changes

mithrandi reviewed Mar 15, 2020

View reviewed changes

glyph added 2 commits April 20, 2020 17:02

Merge branch 'master' into revert-155-revert-140-86-service-stop

9251a42

Update src/txacme/newsfragments/86.bugfix

68bd459

glyph added 6 commits August 25, 2020 13:17

reduce the test matrix to exclude stuff that we know is broken

0a8bd8f

a somewhat unceremonious farewell

804f2f3

YOU ARE THE WEAKEST LINK GOODBYE

c9c0056

"oops"

67c387c

bye 3.5

cfdfc0f

repair coverage damage from dropping py2

b05a7c6

glyph added 10 commits August 25, 2020 17:20

try to cover the remaining coverage gap

e631a61

and hey it looks like stop() didn't actually work right due to the .called check

oooooops

d824a79

pyflakes

a754c13

fixes for overly-prescriptive lint

1059045

more bitrot; work with more recent versions of virtualenv/tox

f31eeab

try a more recent python?

7c7f261

more coverage

138c5ec

coverage fix: test case for the test case

aca0430

flake8

c29e103

more aggressively appease coverage

199858a

glyph merged commit a58740d into master Aug 26, 2020

glyph deleted the revert-155-revert-140-86-service-stop branch August 26, 2020 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close the HTTP pool when service is stopped. Take 2. #156

Close the HTTP pool when service is stopped. Take 2. #156

glyph commented Feb 24, 2020 •

edited by adiroiban

glyph commented Feb 24, 2020

codecov bot commented Feb 24, 2020 •

edited

adiroiban commented Feb 28, 2020

adiroiban commented Feb 28, 2020

glyph commented Feb 28, 2020

glyph commented Feb 28, 2020

adiroiban commented Feb 28, 2020 •

edited

adiroiban commented Mar 13, 2020

glyph commented Mar 13, 2020

adiroiban commented Mar 14, 2020

glyph commented Mar 15, 2020

glyph left a comment

glyph Mar 15, 2020

glyph Mar 15, 2020

adiroiban Aug 18, 2020

glyph Mar 15, 2020

mithrandi Mar 15, 2020

adiroiban Mar 16, 2020

glyph Mar 16, 2020

glyph Mar 15, 2020

mithrandi Mar 15, 2020

adiroiban Mar 16, 2020

glyph commented Aug 17, 2020

adiroiban commented Aug 18, 2020

glyph commented Aug 26, 2020

adiroiban commented Aug 26, 2020

		@@ -19,6 +21,24 @@
		from txacme.util import clock_now, generate_private_key


		class TXACMETestCase(TestCase):

Close the HTTP pool when service is stopped. Take 2. #156

Close the HTTP pool when service is stopped. Take 2. #156

Conversation

glyph commented Feb 24, 2020 • edited by adiroiban

Scope

glyph commented Feb 24, 2020

codecov bot commented Feb 24, 2020 • edited

Codecov Report

adiroiban commented Feb 28, 2020

adiroiban commented Feb 28, 2020

glyph commented Feb 28, 2020

glyph commented Feb 28, 2020

adiroiban commented Feb 28, 2020 • edited

adiroiban commented Mar 13, 2020

glyph commented Mar 13, 2020

adiroiban commented Mar 14, 2020

glyph commented Mar 15, 2020

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

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 commented Aug 17, 2020

adiroiban commented Aug 18, 2020

glyph commented Aug 26, 2020

adiroiban commented Aug 26, 2020

glyph commented Feb 24, 2020 •

edited by adiroiban

codecov bot commented Feb 24, 2020 •

edited

adiroiban commented Feb 28, 2020 •

edited