Skip to content

Make pause/resume_reading idepotent and no-op for closed transports - #528

Closed
fafhrd91 wants to merge 1 commit into
python:masterfrom
fafhrd91:pause-resume-idepotent
Closed

Make pause/resume_reading idepotent and no-op for closed transports#528
fafhrd91 wants to merge 1 commit into
python:masterfrom
fafhrd91:pause-resume-idepotent

Conversation

@fafhrd91

@fafhrd91fafhrd91 commented Mar 6, 2017

Copy link
Copy Markdown
Contributor

@mention-bot

mention-bot commented Mar 6, 2017

Copy link
Copy Markdown

@fafhrd91, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Haypo, @1st1, @tiran, @asvetlov and @benjaminp to be potential reviewers.

@fafhrd91

fafhrd91 commented Mar 6, 2017

Copy link
Copy Markdown
ContributorAuthor

@1st1

1st1
1st1 reviewed Mar 6, 2017
View reviewed changes
Comment threadLib/asyncio/proactor_events.py Outdated

@1st11st1Mar 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

1st1
1st1 reviewed Mar 6, 2017
View reviewed changes
Comment threadLib/test/test_asyncio/test_proactor_events.py Outdated

@1st11st1Mar 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if we need a public Transport.is_reading() method.

@fafhrd91fafhrd91Mar 6, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'd add Transport.paused property instead

@1st11st1Mar 7, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@fafhrd91fafhrd91Mar 7, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@fafhrd91fafhrd91 changed the title Make pause/resume_reading idepotent and no-op during closingMake pause/resume_reading idepotent and no-op for closed transportsMar 7, 2017
@fafhrd91

fafhrd91 commented Mar 7, 2017

Copy link
Copy Markdown
ContributorAuthor

updated debug messages

@1st1 do we still need to support _SelectorSslTransport (legacy)?

@1st1

1st1 commented Mar 7, 2017

Copy link
Copy Markdown
Member

@1st1 do we still need to support _SelectorSslTransport (legacy)?

_SelectorSslTransport is only used in 3.4, right? If yes, then we don't need to care about it.

@fafhrd91

fafhrd91 commented Mar 7, 2017

Copy link
Copy Markdown
ContributorAuthor

that depends on availability of ssl's MemoryBIO. Do you think we should remove code related to old versions of python?

@1st1

1st1 commented Mar 7, 2017

Copy link
Copy Markdown
Member

that depends on availability of ssl's MemoryBIO.

Isn't it always available in 3.5/3.6?

Do you think we should remove code related to old versions of python?

Yeah, I was planning to do a cleanup myself at some point.

@fafhrd91

fafhrd91 commented Mar 12, 2017

Copy link
Copy Markdown
ContributorAuthor

@1st1 ping

1st1
1st1 reviewed Mar 12, 2017
View reviewed changes
Comment threadLib/asyncio/proactor_events.py
raise RuntimeError('Not paused')
self._paused = False
if self._closing:
return

@1st11st1Mar 12, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@fafhrd91fafhrd91May 8, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@fafhrd91fafhrd91 mentioned this pull request Apr 8, 2017
Closed
@1st1

1st1 commented Oct 11, 2017

Copy link
Copy Markdown
Member

Nikolay, could you please do the following and I'll merge this in:

  1. Add ReadTransport.is_paused() method (and implement it for all transports.
  2. Rebase & add a NEWS entry using blurb.

@fafhrd91fafhrd91 mentioned this pull request Oct 19, 2017
Closed
@1st1

1st1 commented Nov 17, 2017

Copy link
Copy Markdown
Member

@asvetlov Can you take a look at this one?

@1st1

1st1 commented Nov 17, 2017

Copy link
Copy Markdown
Member

Add ReadTransport.is_paused() method (and implement it for all transports.

Let's call it Transport.is_reading().

1st1 pushed a commit to MagicStack/uvloop that referenced this pull request Nov 17, 2017
Yury Selivanov
Make Transport.resume_reading() and pause_reading() idempotent. Add i…
e1b7a1a
…s_reading().
Fixes issue #93.
Implements python/cpython#528 asyncio change.
@1st11st1 mentioned this pull request Nov 17, 2017
Merged
@asvetlov

asvetlov commented Nov 17, 2017

Copy link
Copy Markdown
Contributor

The PR branch is gone, the PR should be manually re-created -- as I did for #2269
U have an intention to work on it this weekend.

@asvetlovasvetlov self-assigned this Nov 17, 2017
@1st1

1st1 commented Dec 18, 2017

Copy link
Copy Markdown
Member

Closing this in favour of #4914

@1st11st1 closed this Dec 18, 2017
Olaf1022 pushed a commit to Olaf1022/Ultra-fast-asyncio that referenced this pull request Aug 13, 2023
Yury Selivanov
Make Transport.resume_reading() and pause_reading() idempotent. Add i…
0a147d2
…s_reading().
Fixes issue #93.
Implements python/cpython#528 asyncio change.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@1st11st11st1 left review comments

Assignees

@asvetlovasvetlov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fafhrd91@mention-bot@1st1@asvetlov@Mariatta@the-knights-who-say-ni