Add support for Named Pipes to Site and Connector under Windows #3632
Conversation
hackrush01
force-pushed
the
named-pipes-support
branch
from
2778d5e to
532ba8b
Compare
Codecov Report
@@ Coverage Diff @@
## master #3632 +/- ##
=======================================
Coverage 97.83% 97.83%
=======================================
Files 43 43
Lines 8604 8604
Branches 1377 1377
=======================================
Hits 8418 8418
Misses 80 80
Partials 106 106Continue to review full report at Codecov.
|
|
Thanks.
As per your points, that's exactly what I'm working on right now. :-)
|
hackrush01
force-pushed
the
named-pipes-support
branch
from
6848e46 to
43999c0
Compare
|
Windows proactor event policy was added only in python 3.7 |
hackrush01
force-pushed
the
named-pipes-support
branch
from
ba91ef3 to
7a437be
Compare
|
I don't understand what's up with code cov bot |
|
Now it works |
| @@ -58,7 +57,9 @@ def name(self) -> str: | |||
| self._runner._unreg_site(self) | |||
| return # not started yet | |||
| self._server.close() | |||
| await self._server.wait_closed() | |||
| # named pipes do not have wait_closed property | |||
There was a problem hiding this comment.
Named pipes in asyncio are from the class: https://github.com/python/cpython/blob/master/Lib/asyncio/windows_events.py#L241
Which comprises of only normal sync methods. .close() is a sync method as well. So therefore it has a .closed() property. Are you suggesting to add .wait_closed() property there for consistency(hence a PR to cython)?
Or is it something else?
There was a problem hiding this comment.
Yeah, I'd just inherit that and return immediately maybe. Let's see what @asvetlov thinks.
| loop = asyncio.get_event_loop() | ||
| server = self._runner.server | ||
| assert server is not None | ||
| _server = await loop.start_serving_pipe( # type: ignore |
There was a problem hiding this comment.
start_serving_pipe is not defined in AbstractEventLoop. So it throws the following
aiohttp\connector.py:1149: error: Invalid type "asyncio.ProactorEventLoop"
aiohttp\connector.py:1166: error: "AbstractEventLoop" has no attribute "create_pipe_connection"; ma
ybe "create_connection" or "create_unix_connection"?
Is there some other way I'm missing?
There was a problem hiding this comment.
Dunno. Maybe @asvetlov knows a better way
aiohttp/connector.py
Outdated
| try: | ||
| with CeilTimeout(timeout.sock_connect): | ||
| _, proto = \ | ||
| await self._loop.create_pipe_connection( # type: ignore |
| def __init__(self, path: str, force_close: bool=False, | ||
| keepalive_timeout: Union[object, float, None]=sentinel, | ||
| limit: int=100, limit_per_host: int=0, | ||
| loop: Optional[asyncio.AbstractEventLoop]=None) -> None: |
There was a problem hiding this comment.
| loop: Optional[asyncio.AbstractEventLoop]=None) -> None: | |
| loop: Optional[asyncio.ProactorEventLoop]=None) -> None: |
There was a problem hiding this comment.
Changing this causes(while running mypy)
aiohttp\connector.py:1149: error: Invalid type "asyncio.ProactorEventLoop"
There was a problem hiding this comment.
Odd... are you running mypy under Python 3.7?
There was a problem hiding this comment.
Maybe we could add a conditional for this to work under Linux?
from typing import TYPE_CHECKING
if TYPE_CHECKING:
try:
from asyncio import ProactorEventLoop
except ImportError:
from asyncio import AbstractEventLoop
class ProactorEventLoop(ProactorEventLoop):
"""ProactorEventLoop stub for mypy."""There was a problem hiding this comment.
I am infact running mypy under python 3.7.
Also I too tried with
if TYPE_CHECKING:
from asyncio import ProactorEventLoop
Still the same error.
I'll try again, maybe I did something wrong in rush
There was a problem hiding this comment.
Nope, still happening. It is odd
There was a problem hiding this comment.
It's because you're trying to make the import under Linux and docs say it's available only under windows
There was a problem hiding this comment.
It fails on my computer too, where I'm running it on windows 🤔
There was a problem hiding this comment.
Though, what's surprising is that it failes to find ProactorEventLoop during definition
but not when used inside the functions, such as this instance
assert isinstance(self._loop, asyncio.ProactorEventLoop)
There was a problem hiding this comment.
attribute access is not static analysis by MyPy
| loop - Optional event loop. | ||
| """ | ||
|
|
||
| def __init__(self, path: str, force_close: bool=False, |
There was a problem hiding this comment.
You should validate that it runs under ProactorEventLoop somewhere at the beginning of this method, I think.
There was a problem hiding this comment.
I thought about it, but creating a NamedPipeConnector, raises
AttributeError: '_WindowsSelectorEventLoop' object has no attribute 'create_pipe_connection'
Wouldn't that be okay?
But if there is no assertion then the error wouldn't be raised when loop.create_pipe_connection() is mocked out. So maybe there is a need for the assertion too.
I'd be fine with whatever seems okay to you.
There was a problem hiding this comment.
Also, if we end up adding the assert in this case, we'd be adding it here as well, right?
https://github.com/aio-libs/aiohttp/pull/3632/files#diff-dbf24c78287ef0b964d4945f339a43a9R135
|
I removed a test in |
Co-Authored-By: hackrush01 <[email protected]>
|
Oy Vey! No way to cancel AppVeyor and Travis Builds... |
webknjaz
changed the title
hackrush01
force-pushed
the
named-pipes-support
branch
from
263701d to
c0ec73e
Compare
|
@asvetlov I'm really looking forward to using this - what's holding up merging it? |
|
I need time to review the PR carefully. Do you ask for merging only or for publishing a new aiohttp release with windows named pipes functionality implemented? |
|
Ultimately, releasing a new aiohttp with this feature is the most useful thing. Although just merging it would also help in terms of knowing that this is how it will work / what the API will be like, etc, even if we can't really use it yet (until it's on pypi), we can at least start programming against a future release of it. Would you say that at least the overall API / architecture of this PR is how it will be in the final version? |
|
I'm on the trip now. Will review and merge the PR in a few days |
|
Awesome, thanks! Have a safe trip. |
|
This pull request introduces 1 alert when merging e96f6c6 into ffe04cc - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 5739e1e into ffe04cc - view on LGTM.com new alerts:
|
|
Merged manually |
|
Thanks, @eukreign |
…der Windows. (cherry picked from commit edc7c0f) Co-authored-by: Andrew Svetlov <[email protected]>
…der Windows. (#3885) (cherry picked from commit edc7c0f) Co-authored-by: Andrew Svetlov <[email protected]>
What do these changes do?
Adds support for named pipes in windows
Are there changes in behavior for the user?
No
Related issue number
#3629
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.