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

Add asyncio (tulip, PEP 3156) to stdlib #63461

Closed
gvanrossum opened this issue Oct 14, 2013 · 45 comments
Closed

Add asyncio (tulip, PEP 3156) to stdlib #63461

gvanrossum opened this issue Oct 14, 2013 · 45 comments
Labels
release-blocker type-feature A feature request or enhancement

Comments

@gvanrossum
Copy link
Member

BPO 19262
Nosy @gvanrossum, @birkenfeld, @pitrou, @vstinner, @larryhastings, @giampaolo, @benjaminp, @ned-deily, @skrah, @serhiy-storchaka, @koobs
Files
  • asyncio2.patch
  • asyncio3.patch
  • asyncio4.patch
  • asyncio5.patch
  • asyncio6.patch
  • asyncio7.patch
  • asyncio8.patch
  • koobs-freebsd9-py3x-build178.log
  • koobs-freebsd9-py3x-build180.log
  • koobs-freebsd9-py3x-build182.log
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2013-10-18.22:31:56.872>
    created_at = <Date 2013-10-14.21:18:19.003>
    labels = ['type-feature', 'release-blocker']
    title = 'Add asyncio (tulip, PEP 3156) to stdlib'
    updated_at = <Date 2013-10-19.05:00:06.847>
    user = 'https://github.com/gvanrossum'

    bugs.python.org fields:

    activity = <Date 2013-10-19.05:00:06.847>
    actor = 'gvanrossum'
    assignee = 'none'
    closed = True
    closed_date = <Date 2013-10-18.22:31:56.872>
    closer = 'gvanrossum'
    components = []
    creation = <Date 2013-10-14.21:18:19.003>
    creator = 'gvanrossum'
    dependencies = []
    files = ['32125', '32127', '32129', '32134', '32140', '32145', '32158', '32198', '32199', '32200']
    hgrepos = []
    issue_num = 19262
    keywords = ['needs review']
    message_count = 45.0
    messages = ['199953', '199954', '199955', '199957', '199959', '199962', '199963', '199965', '199973', '199981', '200006', '200028', '200029', '200030', '200072', '200073', '200075', '200077', '200078', '200079', '200080', '200081', '200082', '200156', '200159', '200161', '200163', '200167', '200179', '200188', '200231', '200235', '200261', '200268', '200283', '200288', '200307', '200309', '200317', '200319', '200320', '200323', '200325', '200363', '200369']
    nosy_count = 14.0
    nosy_names = ['gvanrossum', 'georg.brandl', 'pitrou', 'vstinner', 'larry', 'giampaolo.rodola', 'benjamin.peterson', 'ned.deily', 'skrah', 'python-dev', 'sbt', 'serhiy.storchaka', 'koobs', 'David.Edelsohn']
    pr_nums = []
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'patch review'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue19262'
    versions = ['Python 3.4']

    @gvanrossum
    Copy link
    Member Author

    I'll add the tests next. The plan is to get this in the next (last) alpha release.

    @gvanrossum gvanrossum added release-blocker type-feature A feature request or enhancement labels Oct 14, 2013
    @pitrou
    Copy link
    Member

    pitrou commented Oct 14, 2013

    It will probably need some messaging to integrate the Windows overlapped stuff?

    @pitrou
    Copy link
    Member

    pitrou commented Oct 14, 2013

    (massaging, probably)

    @serhiy-storchaka serhiy-storchaka changed the title Add asyncio (tulip, PEP 3156) to stdlin Add asyncio (tulip, PEP 3156) to stdlib Oct 14, 2013
    @birkenfeld
    Copy link
    Member

    If you need help with docs, let me know.

    @gvanrossum
    Copy link
    Member Author

    Here's an updated patch that doesn't botch the selectors imports.

    @gvanrossum
    Copy link
    Member Author

    What I need help with most right now is a suggestion of where the unittests should go. There are too many of them to all put in one file. Some options:

    asyncio/test/test.py
    asyncio/test/test
    .py
    test/test_asyncio/<either of the above>

    What's the best current practice?

    (I also want to add some hacks so that the files can actually be identical in the stdlib and in the separate 3rd party repo, otherwise keeping the two in sync will be too much work.)

    @pitrou
    Copy link
    Member

    pitrou commented Oct 14, 2013

    asyncio/test/test.py
    asyncio/test/test
    .py
    test/test_asyncio/<either of the above>

    Personally I have a preference for test/test_asyncio/test_*.py, because
    putting all tests in test/ makes them easier to find.

    However, other packages such as unittest have their dedicated test
    packages (unittest/test/...).

    As for test_.py vs. *test.py, test.py is definitely the norm in the
    source tree.

    @gvanrossum
    Copy link
    Member Author

    OK, here's a new patch that includes tests.

    To run the tests, I use:

    ./python.exe Lib/test/regrtest.py --fromfile Lib/test/test_asyncio/tests.txt --verbose

    There are a total of 4 individual test failures, all having to do with SSL:

    ssl.SSLEOFError: EOF occurred in violation of protocol (_ssl.c:589)

    Did anything change in the ssl module recently?

    @gvanrossum
    Copy link
    Member Author

    Found the cause of the ssl test failure -- the location of the ssl test key and cert are different. Here's a new patch with a quick fix (#4), but I think the correct solution is to either have the certificates inline in the source and write them to a temp file, or move them into the main asyncio library, or move the test_utils.py module -- since the test files are referenced from test_utils.py they should be in the same directory. Preferences?

    There are still some test failures, the Windows tests are being run everywhere. I'll look into how to do that later.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 15, 2013

    Found the cause of the ssl test failure -- the location of the ssl
    test key and cert are different. Here's a new patch with a quick
    fix (#4), but I think the correct solution is to either have the
    certificates inline in the source and write them to a temp file, or
    move them into the main asyncio library, or move the test_utils.py
    module -- since the test files are referenced from test_utils.py
    they should be in the same directory. Preferences?

    You could simply reuse Lib/test/keycert.pem (when in stdlib mode).

    @gvanrossum
    Copy link
    Member Author

    Turns out there were other uses of the sample key/cert pair. The easiest solution is to just have the code try both locations if necessary. Here's a new patch to review.

    @gvanrossum
    Copy link
    Member Author

    Here's a partial patch for Windows. (Mostly for myself; I need to integrate this into the main patch.)

    @gvanrossum
    Copy link
    Member Author

    Here's a full new patch, with Windows project/solution changes included, and updated from the latest Tulip asyncio branch.

    @gvanrossum
    Copy link
    Member Author

    PS. There's some garbage at the start of pcbuild.sln (perhaps a BOM mark?). I'm not going to re-upload the patch for now, but please note this.

    @gvanrossum
    Copy link
    Member Author

    New patch, mostly SSL hardening IIRC.

    @gvanrossum
    Copy link
    Member Author

    I could use some help for two issues with the tests:

    (1) How do I stop regrtest.py from running the windows tests? (These import _winapi.)
    2 tests failed:
    test_asyncio.test_windows_events test_asyncio.test_windows_utils

    (2) I get this message -- what does it mean and should I care?
    2 tests altered the execution environment:
    test_asyncio.test_base_events test_asyncio.test_futures

    @ned-deily
    Copy link
    Member

    1. The test case decorator perhaps:

    @unittest.skipIf(sys.platform == "win32", "Does not apply to Windows")

    @ned-deily
    Copy link
    Member

    1. See class saved_test_environment in regrtest.py. I wouldn't worry too much about it initially but it should be looked at and tidied up before release.

    @sbt
    Copy link
    Mannequin

    sbt mannequin commented Oct 16, 2013

    On 16/10/2013 8:14pm, Guido van Rossum wrote:

    (2) I get this message -- what does it mean and should I care?
    2 tests altered the execution environment:
    test_asyncio.test_base_events test_asyncio.test_futures

    Perhaps threads from the ThreadExecutor are still alive when those tests
    finish.

    @gvanrossum
    Copy link
    Member Author

    I'd have to decorate a lot of tests. Is there a way to fix this at the module or at least class level? (I'd be willing to move the imports around.)

    @sbt
    Copy link
    Mannequin

    sbt mannequin commented Oct 16, 2013

    I think at module level you can do

         if sys.platform != 'win32':
             raise unittest.SkipTest('Windows only')

    @gvanrossum
    Copy link
    Member Author

    Yup, that works! Not uploading a new patch right now but this is in the tulip repo now.

    @benjaminp
    Copy link
    Contributor

    You can skip classes with skipIf as a class decorator.

    @gvanrossum
    Copy link
    Member Author

    Another day, another patch.

    I'd rather like to commit this (and then iterate as needed), it makes my workflow for porting to Windows a little easier.

    Larry???

    @larryhastings
    Copy link
    Contributor

    Does it break anything? (Besides, possibly, itself?)

    @gvanrossum
    Copy link
    Member Author

    It doesn't break anything AFAICT. (It's a pure addition except for one new
    extension module on Windows).

    However I just discovered that apparently regrtest doesn't automatically
    run tests in a subdirectory of the test package. I'm guessing I'll need to
    add some magic to test_asyncio/init.py. Any hints?

    On Thu, Oct 17, 2013 at 12:59 PM, Larry Hastings <report@bugs.python.org>wrote:

    Larry Hastings added the comment:

    Does it break anything? (Besides, possibly, itself?)

    ----------


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue19262\>


    @larryhastings
    Copy link
    Contributor

    If its breakage is restricted to itself (and its tests) then you have my blessing to check it in. (Sorry, can't help you with the test expertise.)

    @gvanrossum
    Copy link
    Member Author

    OK, no more giant patches. It's checked in. I've also solved the regrtest issues by adding some code in __init__.py and __main__.py.

    I expect to iterate a bit over the next few days.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 18, 2013

    New changeset 30f33e6a04c1 by Ned Deily in branch 'default':
    Issue bpo-19262: Install asyncio and test_asyncio directories.
    http://hg.python.org/cpython/rev/30f33e6a04c1

    @koobs
    Copy link

    koobs commented Oct 18, 2013

    There are 5 unique test failures that have come up in the koobs-freebsd* buildbots post the test_asyncio import. Would we prefer to create a meta issue to track them, or put them here?

    @koobs
    Copy link

    koobs commented Oct 18, 2013

    Summary of 4 test failures below, will attach the complete buildbot logs for detail.

    ======================================================================
    FAIL: test_call_later (test.test_asyncio.test_events.SelectEventLoopTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/usr/home/buildbot/python/3.x.koobs-freebsd9/build/Lib/test/test_asyncio/test_events.py", line 241, in test_call_later
        self.assertTrue(0.09 <= t1-t0 <= 0.12, t1-t0)
    AssertionError: False is not true : 0.12008954107295722

    ======================================================================
    FAIL: test_signal_handling_args (test.test_asyncio.test_events.SelectEventLoopTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/usr/home/buildbot/python/3.x.koobs-freebsd9/build/Lib/test/test_asyncio/test_events.py", line 468, in test_signal_handling_args
        self.assertEqual(caught, 1)
    AssertionError: 0 != 1

    ======================================================================
    FAIL: test_create_server_ssl (test.test_asyncio.test_events.KqueueEventLoopTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/usr/home/buildbot/python/3.x.koobs-freebsd9/build/Lib/test/test_asyncio/test_events.py", line 626, in test_create_server_ssl
        self.assertEqual(3, proto.nbytes)
    AssertionError: 3 != 0

    ======================================================================
    Timeout (1:00:00)!
    <snip>
    Exception: Child error on test_asyncio: Exit code 1
    ----------------------------------------------------------------------

    @pitrou
    Copy link
    Member

    pitrou commented Oct 18, 2013

    ======================================================================
    FAIL: test_call_later
    (test.test_asyncio.test_events.SelectEventLoopTests)
    ----------------------------------------------------------------------

    > Traceback (most recent call last):
    >   File
    >   "/usr/home/buildbot/python/3.x.koobs-freebsd9/build/Lib/test/test_asyncio/test_events.py",
    >   line 241, in test_call_later
    >     self.assertTrue(0.09 <= t1-t0 <= 0.12, t1-t0)
    > AssertionError: False is not true : 0.12008954107295722

    This one is a classical timing issue. The test is too optimistic:
    many buildbots can be quite slow or loaded. Other timing tests in the
    stdlib allow for much more slack.

    (e.g. call_later with 0.5 and check that the resulting delay
    is between 0.4 and 1.0)

    @gvanrossum
    Copy link
    Member Author

    @koobs: I'll look into these, but in the future it's better to report bugs "upstream" for now, i.e. at http://code.google.com/p/tulip/ -- they will get my immediate attention.

    @antoine: while most of the timing-related tests use a simulated clock, there are still some that must use real time. I'll fix them as needed.

    @gvanrossum
    Copy link
    Member Author

    I fixed the easy one (the expected delay in test_call_later). I could use some hands with the rest -- I suspect there are similar race conditions.

    I'm tracking this now in http://code.google.com/p/tulip/issues/detail?id=75

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Oct 18, 2013

    The --without-threads buildbot fails, so I guess all tests need to be
    skipped in that case:

    http://buildbot.python.org/all/builders/AMD64%20Fedora%20without%20threads%203.x/builds/5333/steps/test/logs/stdio

    @gvanrossum
    Copy link
    Member Author

    Maybe adding something that returns [] from suite() if therea re no threads in test/test_asyncio/init.py would help? I don't have time to test this, but go ahead and commit something if it's a release blocker.

    Even better would or course be to fix asyncio to actually work if there are no threads -- the only issue is what to do about run_in_executor(), I guess it will have to run the function in-line...

    @DavidEdelsohn
    Copy link
    Mannequin

    DavidEdelsohn mannequin commented Oct 18, 2013

    AIX buildbot is experiencing a similar failure:

    [276/382/4] test_asyncio
    Timeout (1:00:00)!
    Thread 0x00000001:
    File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/selectors.py", line 265 in select
    File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/asyncio/base_events.py", line 576 in _run_once
    File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/asyncio/base_events.py", line 153 in run_forever
    File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/asyncio/base_events.py", line 172 in run_until_complete
    File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/test/test_asyncio/test_events.py", line 1012 in test_subprocess_interactive

    @koobs
    Copy link

    koobs commented Oct 18, 2013

    @guido, another expected delay test failure:

    ======================================================================
    FAIL: test_run_until_complete (test.test_asyncio.test_events.KqueueEventLoopTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/usr/home/buildbot/python/3.x.koobs-freebsd9/build/Lib/test/test_asyncio/test_events.py", line 218, in test_run_until_complete
        self.assertTrue(0.08 <= t1-t0 <= 0.12, t1-t0)
    AssertionError: False is not true : 0.12018337799236178

    Note: Also referenced "upstream" :]

    @gvanrossum
    Copy link
    Member Author

    Relaxed a bunch of timeouts. No news on the --without-threads case, that
    will have to happen post-alpha-4 I expect.

    @gvanrossum
    Copy link
    Member Author

    I've landed a bunch of stuff, and now I am pretty happy. It's also soon going to be weekend, which means family time, so I really hope everyone else is also happy. :-)

    A summary of what changed since the initial asyncio checkin:

    • Rename the logger to plain "logger".
    • Rename Transport.pause/resume to pause_reading/pause_writing.
    • Important race condition fix.
    • Write flow control for asyncio! (And asyncio/streams.py overhaul.)

    And in the tests:

    • Make the tests run on Windows.
    • Relax various test timeouts to reduce flakiness on slow buildbots.

    @gvanrossum
    Copy link
    Member Author

    Ready for release!

    @vstinner
    Copy link
    Member

    2013/10/19 Guido van Rossum <report@bugs.python.org>:

    Ready for release!

    resolution: -> fixed
    status: open -> closed

    The new module has no documentation at all. Do you plan to open a new
    issue for the documentation? Or maybe you don't plan to add
    documentation? :-)

    @gvanrossum
    Copy link
    Member Author

    I'll track that separately: http://bugs.python.org/issue19291

    For now, PEP-3156 has a lot of information. (But the PEP also needs to be updated to track recent developments.)

    @DavidEdelsohn
    Copy link
    Mannequin

    DavidEdelsohn mannequin commented Oct 19, 2013

    How is this ready for release? The patch does not work on numerous POSIX systems.

    @gvanrossum
    Copy link
    Member Author

    Please be more specific.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    release-blocker type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants