Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.8] bpo-38242: Revert new asyncio streaming API #16445

Closed
wants to merge 8 commits into from

Conversation

aeros
Copy link
Contributor

@aeros aeros commented Sep 27, 2019

Motivation: Comment in bpo issue from @1st1:

My final opinion on this issue: we must revert the new streaming API from asyncio and take our time to design it properly. I don't like what we have in the 3.8 branch right now. I don't feel comfortable rushing decisions and doing last minute API changes.

Reverts the following commits from 3.8:

Merge asyncio streams (primary reversion):
23b4b69

Documentation:
b9bfe14
7268fd0
ed9f356

Minor test changes (incompatible with asyncio.StreamReader and asyncio.StreamWriter):
4cdbc452ce3

Manual resolution of several conflicts was also required, the majority were for retaining the the deprecation of explicitly passing the loop argument for tasks (345bfc9). There were a few others as well.

This PR must be carefully reviewed, especially since it is removing multiple tests that no longer function properly without the changes made in 23b4b69. It is important to be 100% certain that no additional tests were removed. I believe @asvetlov would be the most qualified individual to verify the tests are in a correct state, since he wrote the majority of the tests related to the new streaming API.

I believe it is most of the way there, but additional changes may be needed before it is complete. Until we are certain, I am adding the DO-NOT-MERGE label.

To @1st1 and @asvetlov:

I have enabled "Allow edits from maintainers" (as I usually do). Please feel free to directly commit changes into the PR. I would appreciate review feedback so that I can understand the reasons behind any changes made, but you certainly don't have to wait on me to make any changes to the PR. Especially since this is a release blocker for 3.8.0.

https://bugs.python.org/issue38242

@1st1
Copy link
Member

1st1 commented Sep 27, 2019

Kyle, the PR should be against the master branch. We can then backport it to 3.8.

.. coroutinefunction:: start_server(client_connected_cb, host=None, \
port=None, \*, loop=None, limit=2**16, \
port=None, \*, loop=None, limit=None, \
Copy link
Contributor Author

@aeros aeros Sep 27, 2019

Choose a reason for hiding this comment

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

Can the change to the default value of limit be retained? I wasn't sure about this one, so I figured it would be best to ask first.

@aeros aeros changed the title bpo-38242: [3.8] Revert new asyncio streaming API [3.8] bpo-38242: Revert new asyncio streaming API Sep 27, 2019
@aeros
Copy link
Contributor Author

aeros commented Sep 28, 2019

All merge conflicts have been resolved.

@@ -30,27 +30,21 @@ def check_all(self, modname):
raise NoAll(modname)
names = {}
with self.subTest(module=modname):
with support.check_warnings(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1st1 can the changes from lines 33-53 remain? This is unrelated to the streaming API.

@aeros
Copy link
Contributor Author

aeros commented Sep 28, 2019

@1st1:

Kyle, the PR should be against the master branch. We can then backport it to 3.8.

Are you certain? The conflicts in 3.9 will probably take significantly longer to resolve (since there will likely be additional ones), so if we want to get the patch through ASAP it might be better to do it manually against 3.8 first.

@aeros
Copy link
Contributor Author

aeros commented Sep 28, 2019

I'll close this one and create a new one against 3.9. It should be a bit easier since I figured out exactly what commits to revert. Let me know if this one should be reopened.

@aeros aeros closed this Sep 28, 2019
@1st1
Copy link
Member

1st1 commented Sep 28, 2019

I'll close this one and create a new one against 3.9. It should be a bit easier since I figured out exactly what commits to revert. Let me know if this one should be reopened.

Thanks. We want to keep 3.8 and master as much in sync as possible.

@aeros
Copy link
Contributor Author

aeros commented Sep 28, 2019

New PR: #16455

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants