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

Port http/ to Python 3 #3771

Merged
merged 22 commits into from
Sep 5, 2018
Merged

Port http/ to Python 3 #3771

merged 22 commits into from
Sep 5, 2018

Conversation

hawkowl
Copy link
Contributor

@hawkowl hawkowl commented Aug 30, 2018 •
edited
Loading

This shifts us over to using treq, which bridges a lot of the messy gaps between py2 and py3 for us.

@hawkowl hawkowl requested a review from a team August 30, 2018 14:26
erikjohnston
erikjohnston previously requested changes Sep 3, 2018
View reviewed changes
synapse/python_dependencies.py Outdated
@@ -40,6 +40,7 @@
"pynacl>=1.2.1": ["nacl>=1.2.1", "nacl.bindings"],
"service_identity>=1.0.0": ["service_identity>=1.0.0"],
"Twisted>=17.1.0": ["twisted>=17.1.0"],
"treq>=17.8.0": ["treq>=17.8.0"],
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to make you sad by saying that the only debian packages are 15.1.0

synapse/http/client.py Outdated
data = None

request_deferred = treq.request(
method, uri, *args, agent=self.agent, data=data, **kwargs
Copy link
Member

Choose a reason for hiding this comment

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

I think at this point its going to be cleaner if we get rid of *args and **kwargs and be explicit in what we expect, which I think is just headers and data.

synapse/http/client.py Outdated
request_deferred = self.agent.request(
method, uri, *args, **kwargs
if "data" in kwargs:
data = kwargs.pop("data")
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to do the right thing? We explicitly moved to using a producer to ensure things didn't time out.

synapse/http/client.py Outdated
if "data" in kwargs:
data = kwargs.pop("data")
elif "bodyProducer" in kwargs:
data = kwargs.pop("bodyProducer")
Copy link
Member

Choose a reason for hiding this comment

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

Can we just get rid of bodyProducer arg entirely and replace it with data?

@hawkowl hawkowl requested a review from erikjohnston September 4, 2018 15:11
krombel
krombel reviewed Sep 5, 2018
View reviewed changes
setup.cfg
# doesn't like it. E203 is contrary to PEP8.
ignore = W503,E203
# doesn't like it. E203 is contrary to PEP8. E731 is silly.
ignore = W503,E203,E731
Copy link
Contributor

Choose a reason for hiding this comment

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

You are not doing anything against E731 (BTW: Do not assign lambda and use def instead)
So why do you want to add it here?

krombel
krombel reviewed Sep 5, 2018
View reviewed changes
setup.cfg

[flake8]
# note that flake8 inherits the "ignore" settings from "pep8" (because it uses
# pep8 to do those checks), but not the "max-line-length" setting
max-line-length = 90
ignore=W503,E203,E731
Copy link
Contributor

Choose a reason for hiding this comment

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

based on the comment above that is inherited from pep8 so this is duplicate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I put E731 in pep8 it gets ignored (since it's pyflakes?), if I just put E731 in flake8 it stops ignoring the ones in pep8 :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Although it is merged now, I meant that pep8 currently has the same line. So no "just E731". The whole line could be go away if I get it right.

Copy link
Member

Choose a reason for hiding this comment

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

@hawkowl it would have been nice to fix the comment if you thought it was wrong :/.

@hawkowl hawkowl merged commit 2d2828d into develop Sep 5, 2018
@hawkowl hawkowl deleted the hawkowl/py3-http branch September 5, 2018 14:10
hawkowl added a commit that referenced this pull request Sep 24, 2018
@hawkowl
Merge tag 'v0.33.5'
8292135 
Features
--------

- Python 3.5 and 3.6 support is now in beta.
([\#3576](#3576))
- Implement `event_format` filter param in `/sync`
([\#3790](#3790))
- Add synapse_admin_mau:registered_reserved_users metric to expose
number of real reaserved users
([\#3846](#3846))

Bugfixes
--------

- Remove connection ID for replication prometheus metrics, as it creates
a large number of new series.
([\#3788](#3788))
- guest users should not be part of mau total
([\#3800](#3800))
- Bump dependency on pyopenssl 16.x, to avoid incompatibility with
recent Twisted.
([\#3804](#3804))
- Fix existing room tags not coming down sync when joining a room
([\#3810](#3810))
- Fix jwt import check
([\#3824](#3824))
- fix VOIP crashes under Python 3 (#3821)
([\#3835](#3835))
- Fix manhole so that it works with latest openssh clients
([\#3841](#3841))
- Fix outbound requests occasionally wedging, which can result in
federation breaking between servers.
([\#3845](#3845))
- Show heroes if room name/canonical alias has been deleted
([\#3851](#3851))
- Fix handling of redacted events from federation
([\#3859](#3859))
-  ([\#3874](#3874))
- Mitigate outbound federation randomly becoming wedged
([\#3875](#3875))

Internal Changes
----------------

- CircleCI tests now run on the potential merge of a PR.
([\#3704](#3704))
- http/ is now ported to Python 3.
([\#3771](#3771))
- Improve human readable error messages for threepid
registration/account update
([\#3789](#3789))
- Make /sync slightly faster by avoiding needless copies
([\#3795](#3795))
- handlers/ is now ported to Python 3.
([\#3803](#3803))
- Limit the number of PDUs/EDUs per federation transaction
([\#3805](#3805))
- Only start postgres instance for postgres tests on Travis CI
([\#3806](#3806))
- tests/ is now ported to Python 3.
([\#3808](#3808))
- crypto/ is now ported to Python 3.
([\#3822](#3822))
- rest/ is now ported to Python 3.
([\#3823](#3823))
- add some logging for the keyring queue
([\#3826](#3826))
- speed up lazy loading by 2-3x
([\#3827](#3827))
- Improved Dockerfile to remove build requirements after building
reducing the image size.
([\#3834](#3834))
- Disable lazy loading for incremental syncs for now
([\#3840](#3840))
- federation/ is now ported to Python 3.
([\#3847](#3847))
- Log when we retry outbound requests
([\#3853](#3853))
- Removed some excess logging messages.
([\#3855](#3855))
- Speed up purge history for rooms that have been previously purged
([\#3856](#3856))
- Refactor some HTTP timeout code.
([\#3857](#3857))
- Fix running merged builds on CircleCI
([\#3858](#3858))
- Fix typo in replication stream exception.
([\#3860](#3860))
- Add in flight real time metrics for Measure blocks
([\#3871](#3871))
- Disable buffering and automatic retrying in treq requests to prevent
timeouts. ([\#3872](#3872))
- mention jemalloc in the README
([\#3877](#3877))
- Remove unmaintained "nuke-room-from-db.sh" script
([\#3888](#3888))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@krombel krombel krombel left review comments

@richvdh richvdh richvdh left review comments

@erikjohnston erikjohnston erikjohnston approved these changes

Assignees

@hawkowl hawkowl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hawkowl @richvdh @erikjohnston @krombel