bpo-38314: Add is_reading() method to asyncio _UnixReadPipeTransport #17042

calluw · 2019-11-04T12:24:50Z

Unix read pipe (from Lib:asyncio.unix_events) is missing is_reading() method available for other read transports within the asyncio protocol, which checks that the transport read is not
paused or the transport is not closing.

This behaviour is documented in Doc/library/asyncio-protocol.rst.

Additional tests for the Lib:asyncio.unix_events module which check against the state of the pipe and the behaviour of is_reading().

https://bugs.python.org/issue38314

Unix read pipe is missing method available for other read transports within the aynscio protocol, which checks that the transport read is not paused or the transport is not closing.

calluw · 2019-11-04T12:31:33Z

@aeros Hi Kyle, does this need a news or could you add the skip news label for me? I am contributing as part of the EnHackathon team: https://enhackathon.github.io/.

…on#17043)

asvetlov · 2019-11-04T16:33:14Z

Please avoid mocking but write a test that uses a pair of real sockets connected via loopback interface (localhost).

Mocking is highly not reliable; we have a lot of problems in asyncio behind of this.

P.S.
Yes, NEWs is required for this feature.

asvetlov

Please fix my comments.
The code change itself is correct.

bedevere-bot · 2019-11-04T16:34:44Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

calluw · 2019-11-04T16:58:16Z

Hi @asvetlov, thanks for your comments on the first draft. However, I'd like to ask for some clarification on the changes you've asked for:

Within this test suite, the style of tests is distinct from integration tests like opening sockets into more unit-based tests by mocking the pipe and file descriptors: should we break from this style of tests within the same test suite?
Also, I am not sure how is_reading() would translate in the socket opening integration test asked for when it just checks internal state: its functionality is constrained/verified by the current unit test.

If the requested changes are needed for merge, would you be able to give some guidance on making them?
This is my first contribution PR so apologies if I have misunderstood something.

asvetlov · 2019-11-04T18:12:50Z

The asyncio test suite is in the middle of translation from primarily mock-based tests to almost solely functional ones (this is our target).

Maybe a separate test case in the same file fits better to our needs. Currently, there are a lot of functional tests inside test_events.py; it makes the test file unmaintainable.

That's why unit test pipes better to sit in test_unix_events.py (or even not existing yet test_unix_pipes.py).

The test scenario could be:

Create a pipe pair.
Create a pipe reader (loop.connect_read_pipe())
Check pipe.is_reading()
Call pipe.pause_reading()
Check pipe.is_reading()
Call pipe.resume_reading()
Check pipe.is_reading()
Call pipe.close()
Check pipe.is_reading()

Something like this.

…7046) Provide Py_EnterRecursiveCall() and Py_LeaveRecursiveCall() as regular functions for the limited API. Previously, there were defined as macros, but these macros didn't work with the limited API which cannot access PyThreadState.recursion_depth field. Remove _Py_CheckRecursionLimit from the stable ABI. Add Include/cpython/ceval.h header file.

aeros · 2019-11-04T19:06:57Z

@CallumQuick

Hi Kyle, does this need a news or could you add the skip news label for me?

I think this one could benefit from a brief news entry since it's implementing a new public method (even if the new method is fairly simple and within a private class). I'll add an asyncio label though.

Misc/NEWS.d/next/Library/2019-11-04-17-20-43.bpo-38314.zWz6_P.rst

Co-Authored-By: Kyle Stanley <aeros167@gmail.com>

* Add _Py_EnterRecursiveCall() and _Py_LeaveRecursiveCall() which require a tstate argument. * Pass tstate to _Py_MakeRecCheck() and _Py_CheckRecursiveCall(). * Convert Py_EnterRecursiveCall() and Py_LeaveRecursiveCall() macros to static inline functions. _PyThreadState_GET() is the most efficient way to get the tstate, and so using it with _Py_EnterRecursiveCall() and _Py_LeaveRecursiveCall() should be a little bit more efficient than using Py_EnterRecursiveCall() and Py_LeaveRecursiveCall() which use the "slower" PyThreadState_GET().

* Add tstate parameter to _Py_CheckFunctionResult() * Add _PyErr_FormatFromCauseTstate() * Replace PyErr_XXX(...) with _PyErr_XXX(state, ...)

s/pathing/patching/

…acros in ``pythonrun.h``. (pythonGH-17056)

Remove UNUSED macro: use Py_UNUSED() macro instead.

Fixed what seemed to be a weird phrasing.

@encukou

After python#9665, this moves the remaining types in posixmodule to be heap-allocated to make it compatible with PEP384 as well as modifying all the type accessors to fully make the type opaque. The original PR that got messed up a rebase: python#10854. All the issues in that commit have now been addressed since python#11661 got committed. This change also removes any state from the data segment and onto the module state itself. https://bugs.python.org/issue35381 Automerge-Triggered-By: @encukou

@vstinner

Additional note: the `method_check_args` function in `Objects/descrobject.c` is written in such a way that it applies to all kinds of descriptors. In particular, a future re-implementation of `wrapper_descriptor` could use that code. CC @vstinner @encukou https://bugs.python.org/issue37645 Automerge-Triggered-By: @encukou

* Add _PyObject_VectorcallTstate() function: similar to _PyObject_Vectorcall(), but with tstate parameter * Add tstate parameter to _PyObject_MakeTpCall()

…nt to extra data. (pythonGH-16988)

…at() (pythonGH-17101)

)

…onGH-17111)

calluw · 2019-11-12T10:27:46Z

Thanks for the writeup @asvetlov.
I will take a look at writing a functional test for this change today (and start a new "test case" for Unix read transport functional tests generally).

Unix read pipe is missing method available for other read transports within the aynscio protocol, which checks that the transport read is not paused or the transport is not closing.

… fix-issue-38314

calluw · 2019-11-12T16:24:24Z

Hi @asvetlov, I have made the requested changes; please review again.

I added a new file Lib/test/test_asyncio/test_unix_pipe.py which defines a new functional test suite for Unix read transport pipes. The test suite sets up two file handles with a read pipe transport registered on the event loop.

Added a test to the suite for verifying the new behaviour of is_reading() for _UnixReadPipeTransport.

Thanks,
Callum

bedevere-bot · 2019-11-12T16:24:27Z

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

calluw · 2019-11-21T10:42:49Z

@asvetlov, please could you look at the changes requested?

calluw · 2019-12-11T12:18:10Z

@asvetlov, please could you take a look at these updates? Thanks

zware · 2019-12-11T14:05:20Z

@CallumQuick, after a quick look at the changes here, I think this could do with a clean rebase on master in a new PR (I would suggest just force-pushing over this, but apparently GitHub behaves weirdly with force-push in some situations).

calluw · 2019-12-30T13:54:44Z

@asvetlov, @zware: New PR with updated summary at #17755.

Add is_reading() method to _UnixReadPipeTransport

0a4eabb

Unix read pipe is missing method available for other read transports within the aynscio protocol, which checks that the transport read is not paused or the transport is not closing.

calluw requested review from 1st1 and asvetlov as code owners November 4, 2019 12:24

the-knights-who-say-ni added the CLA signed label Nov 4, 2019

bedevere-bot added the awaiting review label Nov 4, 2019

bedevere-bot added awaiting core review and removed awaiting review labels Nov 4, 2019

bpo-38684: haslib: fix build when Blake2 not enabled in OpenSSL (pyth…

6552563

…on#17043)

asvetlov requested changes Nov 4, 2019

View reviewed changes

bedevere-bot removed the awaiting core review label Nov 4, 2019

bedevere-bot added the awaiting changes label Nov 4, 2019

📜🤖 Added by blurb_it.

f7cd057

aeros added the topic-asyncio label Nov 4, 2019

aeros reviewed Nov 4, 2019

View reviewed changes

Misc/NEWS.d/next/Library/2019-11-04-17-20-43.bpo-38314.zWz6_P.rst Outdated Show resolved Hide resolved

calluw and others added 9 commits November 4, 2019 20:40

Update Misc/NEWS.d/next/Library/2019-11-04-17-20-43.bpo-38314.zWz6_P.rst

4ef4d51

Co-Authored-By: Kyle Stanley <aeros167@gmail.com>

bpo-38644: Pass tstate to _Py_CheckFunctionResult() (pythonGH-17050)

1726909

* Add tstate parameter to _Py_CheckFunctionResult() * Add _PyErr_FormatFromCauseTstate() * Replace PyErr_XXX(...) with _PyErr_XXX(state, ...)

Fix a typo in wave module docstring (pythonGH-17009)

25fa3ec

s/pathing/patching/

closes bpo-37633: Reëxport some function compatibility wrappers for m…

62161ce

…acros in ``pythonrun.h``. (pythonGH-17056)

_json.c: use Py_UNUSED() macro (pythonGH-17053)

fbbfcce

Remove UNUSED macro: use Py_UNUSED() macro instead.

Update interpreter.rst (pythonGH-17059)

5e01a65

Fixed what seemed to be a weird phrasing.

vstinner and others added 9 commits November 8, 2019 10:05

bpo-38644: Add _PyObject_VectorcallTstate() (pythonGH-17052)

7e43373

* Add _PyObject_VectorcallTstate() function: similar to _PyObject_Vectorcall(), but with tstate parameter * Add tstate parameter to _PyObject_MakeTpCall()

Clarify amount of dots between package and subpackage (pythonGH-17092)

fc6b1bf

bpo-38635: Simplify decoding the ZIP64 extra field and make it tolera…

e27449d

…nt to extra data. (pythonGH-16988)

Minor readability improvement for argument handling in itertools.repe…

af46450

…at() (pythonGH-17101)

bpo-38761: Register WeakSet as a MutableSet (pythonGH-17104)

84ac437

bpo-38438: Simplify argparse "star nargs" usage. (pythonGH-17106)

a0ed99b

bpo-38771: Explict test for None in code example (pythonGH-17108)

98480ce

bpo-38565: add new cache_parameters method for lru_cache (pythonGH-16916

051ff52

)

bpo-38385: Fix iterator/iterable terminology in statistics docs (pyth…

733b9a3

…onGH-17111)

Callum added 3 commits November 12, 2019 10:34

Add is_reading() method to _UnixReadPipeTransport

1b5aa07

Unix read pipe is missing method available for other read transports within the aynscio protocol, which checks that the transport read is not paused or the transport is not closing.

Add new test case file for UNIX pipe transport functional tests

1dbad62

Merge branch 'fix-issue-38314' of github.com:callumquick/cpython into…

dd3c0d3

… fix-issue-38314

calluw requested review from methane, rhettinger, tiran, vsajip and a team as code owners November 12, 2019 16:21

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 12, 2019

bedevere-bot requested a review from asvetlov November 12, 2019 16:24

calluw mentioned this pull request Dec 30, 2019

bpo-38314: Add is_reading() method to asyncio _UnixReadPipeTransport #17755

Open

calluw closed this Dec 30, 2019

asvetlov mentioned this pull request Apr 10, 2022

Implement unix read_pipe.is_reading() method #82495

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

bpo-38314: Add is_reading() method to asyncio _UnixReadPipeTransport #17042

bpo-38314: Add is_reading() method to asyncio _UnixReadPipeTransport #17042

Uh oh!

calluw commented Nov 4, 2019 •

edited by bedevere-bot

Loading

Uh oh!

calluw commented Nov 4, 2019

Uh oh!

asvetlov commented Nov 4, 2019

Uh oh!

asvetlov left a comment

Uh oh!

bedevere-bot commented Nov 4, 2019

Uh oh!

calluw commented Nov 4, 2019

Uh oh!

asvetlov commented Nov 4, 2019

Uh oh!

aeros commented Nov 4, 2019 •

edited

Loading

Uh oh!

Uh oh!

calluw commented Nov 12, 2019

Uh oh!

calluw commented Nov 12, 2019

Uh oh!

bedevere-bot commented Nov 12, 2019

Uh oh!

calluw commented Nov 21, 2019

Uh oh!

calluw commented Dec 11, 2019

Uh oh!

zware commented Dec 11, 2019

Uh oh!

calluw commented Dec 30, 2019

Uh oh!

Uh oh!

Uh oh!

bpo-38314: Add is_reading() method to asyncio _UnixReadPipeTransport #17042

bpo-38314: Add is_reading() method to asyncio _UnixReadPipeTransport #17042

Uh oh!

Conversation

calluw commented Nov 4, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calluw commented Nov 4, 2019

Uh oh!

asvetlov commented Nov 4, 2019

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 4, 2019

Uh oh!

calluw commented Nov 4, 2019

Uh oh!

asvetlov commented Nov 4, 2019

Uh oh!

aeros commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

calluw commented Nov 12, 2019

Uh oh!

calluw commented Nov 12, 2019

Uh oh!

bedevere-bot commented Nov 12, 2019

Uh oh!

calluw commented Nov 21, 2019

Uh oh!

calluw commented Dec 11, 2019

Uh oh!

zware commented Dec 11, 2019

Uh oh!

calluw commented Dec 30, 2019

Uh oh!

Uh oh!

calluw commented Nov 4, 2019 •

edited by bedevere-bot

Loading

aeros commented Nov 4, 2019 •

edited

Loading