Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove redundant WrappedConnection #4409

Merged
merged 3 commits into from
Jan 18, 2019
Merged

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Jan 16, 2019

The matrix federation client uses an HTTP connection pool, which times out its idle HTTP connections, so there is no need for any of this business.

The matrix federation client uses an HTTP connection pool, which times out its
idle HTTP connections, so there is no need for any of this business.
@richvdh richvdh requested a review from a team January 16, 2019 23:12
@@ -320,23 +320,23 @@ def _send_request(
url_str,
)

# we don't want all the fancy cookie and redirect handling that
# treq.request gives: just use the raw Agent.
request_deferred = self.agent.request(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some reason, the change makes this now throw some exceptions synchronously, hence moving it inside the try/catch. It should have been there anyway imho.

@@ -63,7 +66,7 @@ def test_client_never_connect(self):
self.pump()

# Nothing happened yet
self.assertFalse(d.called)
self.assertNoResult(d)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this gives a slightly more helpful error when it fails.

@codecov-io
Copy link

codecov-io commented Jan 16, 2019

Codecov Report

Merging #4409 into develop will decrease coverage by 0.03%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4409      +/-   ##
===========================================
- Coverage    73.65%   73.62%   -0.04%     
===========================================
  Files          300      300              
  Lines        29815    29785      -30     
  Branches      4897     4896       -1     
===========================================
- Hits         21961    21929      -32     
- Misses        6412     6413       +1     
- Partials      1442     1443       +1
Impacted Files Coverage Δ
synapse/http/matrixfederationclient.py 84.03% <100%> (ø) ⬆️
synapse/http/endpoint.py 61.34% <50%> (-7.74%) ⬇️
synapse/handlers/device.py 79.68% <0%> (-0.97%) ⬇️
synapse/handlers/user_directory.py 71.08% <0%> (-0.31%) ⬇️
synapse/config/key.py 67.64% <0%> (ø) ⬆️
synapse/app/homeserver.py 57.33% <0%> (+0.33%) ⬆️

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 7f1a6a4...04ac5d3. Read the comment docs.



@implementer(ITransport)
class MockTransport(object):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe https://twistedmatrix.com/documents/current/api/twisted.test.proto_helpers.StringTransport.html would reduce some duplication here? (twisted.test.proto_helpers is public API)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes :)

rather than rolling our own test transport
@richvdh richvdh requested a review from a team January 17, 2019 12:40
@richvdh
Copy link
Member Author

richvdh commented Jan 17, 2019

@hawkowl: I was wondering about adding TLS into the tests, so that we could test the behaviour including the TLS layer. I started looking at Factories and Transports and Protocols, but soon found myself in a maze of twisty little passages, all alike. Have you any insights as to how one might do that?

@hawkowl hawkowl merged commit de6888e into develop Jan 18, 2019
@hawkowl hawkowl deleted the rav/remove_wrapped_connection branch January 18, 2019 12:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants