Make pause/resume_reading idepotent and no-op for closed transports#528
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mention-bot commented Mar 6, 2017
fafhrd91 commented Mar 6, 2017
Lib/asyncio/proactor_events.py Outdated
There was a problem hiding this comment.
I'd change these debug messages to more descriptive ones:
%r not paused->%r: ignoring resume_reading() call; already reading%r already paused->%r: ignoring pause_reading() call; already paused
There was a problem hiding this comment.
I wonder if we need a public Transport.is_reading() method.
There was a problem hiding this comment.
I'd add Transport.paused property instead
There was a problem hiding this comment.
We usually prefer methods to properties: see Transport.is_closing() for instance. Anyways, do you use _paused attribute anywhere in aiohttp? Maybe we should create a separate issue to discuss this.
There was a problem hiding this comment.
aiohttp maintains paused state separately from asyncio transport. it would be useful to have public api for that.
in aiohttp paused state is maintained by different edges, protocol pauses transport
when it get enough data, but resume happen from other edge, when user reads-out data from incoming stream. that is the reason why I think pause/resume should be ideponent and no-op during closing stage.
fafhrd91 changed the title fafhrd91 commented Mar 7, 2017
updated debug messages @1st1 do we still need to support _SelectorSslTransport (legacy)? |
1st1 commented Mar 7, 2017
|
fafhrd91 commented Mar 7, 2017
that depends on availability of ssl's |
1st1 commented Mar 7, 2017
Isn't it always available in 3.5/3.6?
Yeah, I was planning to do a cleanup myself at some point. |
fafhrd91 commented Mar 12, 2017
@1st1 ping |
| raise RuntimeError('Not paused') | ||
| self._paused = False | ||
| if self._closing: | ||
| return |
There was a problem hiding this comment.
I understand why you want to remove the exception from pause_reading. But rising an error on trying to resume_reading on a closed transport seems reasonable.
There was a problem hiding this comment.
I think resume_reading is more important. from my experience pause_reading is getting called from protocol, and protocol has access to transport object, but resume_reading usually happen from higher level, from coroutine side. and coroutine usually doesn't have access to transport or protocol directly, it just writes to some kind of data stream.
1st1 commented Oct 11, 2017
Nikolay, could you please do the following and I'll merge this in:
|
1st1 commented Nov 17, 2017
@asvetlov Can you take a look at this one? |
1st1 commented Nov 17, 2017
Let's call it |
…s_reading(). Fixes issue #93. Implements python/cpython#528 asyncio change.
asvetlov commented Nov 17, 2017
The PR branch is gone, the PR should be manually re-created -- as I did for #2269 |
1st1 commented Dec 18, 2017
Closing this in favour of #4914 |
…s_reading(). Fixes issue #93. Implements python/cpython#528 asyncio change.
6 participants
PR for python/asyncio#488