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

bpo-30300: A convenient "controller" for asyncio-based servers #1492

Closed
wants to merge 5 commits into from

Conversation

warsaw
Copy link
Member

@warsaw warsaw commented May 7, 2017

This is refactored out of aiosmtpd and made generic.

https://bugs.python.org/issue30300

@mention-bot
Copy link

@warsaw, thanks for your PR! By analyzing the history of the files in this pull request, we identified @terryjreedy, @birkenfeld and @eliben to be potential reviewers.

@warsaw warsaw changed the title A convenient "controller" for asyncio-based servers bpo-30300 A convenient "controller" for asyncio-based servers May 7, 2017
try:
from socket import socketpair
except ImportError:
from asyncio.windows_utils import socketpair
Copy link
Member

Choose a reason for hiding this comment

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

No more needed in Python 3.7, just remove it. But i don't know if Controller would only go to asyncio of Python 3.7?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since it's only needed in Python <= 3.4, I'll remove it. Even if it's needed in other places, I don't care about 3.4 any more.

self.ready_timeout = ready_timeout if envar is None else float(envar)
# For exiting the loop.
self._rsock, self._wsock = socketpair()
self.loop.add_reader(self._rsock, self._reader)
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 not sure that a socketpair is really needed here. Why not simply using call_soon_threadsafe() from stop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think that's better. Thanks!

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@terryjreedy
Copy link
Member

Something seems wrong. The IDLE patches have already been merged into cpython/master.

@warsaw
Copy link
Member Author

warsaw commented May 29, 2017

Yikes, something definitely must have gone wrong with the merge of master.

@warsaw
Copy link
Member Author

warsaw commented May 29, 2017

Obviously, I've signed the CLA.

@zware
Copy link
Member

zware commented Jun 10, 2017

I think a rebase and force-push is in order at this point; the actual change is drowned out by changes that GitHub shouldn't be showing on the Files changed tab.

@warsaw
Copy link
Member Author

warsaw commented Jun 14, 2017

@zware Yep. rebase & force pushed.

@zware zware requested review from 1st1 and asvetlov February 4, 2018 20:27
@asvetlov asvetlov changed the title bpo-30300 A convenient "controller" for asyncio-based servers bpo-30300: A convenient "controller" for asyncio-based servers Feb 4, 2018
@asvetlov
Copy link
Contributor

asvetlov commented Feb 4, 2018

Close the PR as https://bugs.python.org/issue30300 was rejected

@asvetlov asvetlov closed this Feb 4, 2018
@zware
Copy link
Member

zware commented Feb 4, 2018

@asvetlov Sorry for the noise, I failed to check the issue.

@asvetlov
Copy link
Contributor

asvetlov commented Feb 4, 2018

No problem

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

8 participants