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

various small fixes #264

Merged
merged 5 commits into from
May 30, 2024
Merged

various small fixes #264

merged 5 commits into from
May 30, 2024

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented May 29, 2024

Various small fixes I noticed while going through code/doc/issues.

  • doc updates to 113 and startable-in-context-manager
  • ASYNC102 await-in-finally-or-cancelled no longer treats open_nursery as a cancel scope.
    • While you can make the cancelscope inside the nursery shielded with a timeout, that was never actually supported (there was no logic for seeing nursery.cancelscope.shield = True, only for nursery.shield = True, which is not possible)
    • open_nursery is itself an async function so you can't safely open a nursery anyway.
  • async112 error message now specifies if it's a nursery or taskgroup.
    • for dumb reasons I confused myself into thinking async111 & async112 didn't have asyncio support, and fully rewrote that from scratch 🙃 But some of the stuff was better so I kept it after noticing my mistake and rebasing onto an updated main.

…way). async112 error message now specifies if its nursery or taskgroup.
@jakkdl
Copy link
Member Author

jakkdl commented May 29, 2024

Don't think this needs a version/changelog bump, but it would probably be good to have some check for if the changelog is updated in a pull request as that's been missed in the past.

@jakkdl
Copy link
Member Author

jakkdl commented May 29, 2024

I think the coverage failure on py39 is a bug in pytest-cov or coverage (??). I can reproduce it locally when running through tox. It's triggering here on line 130:

else:
continue

  • inserting a print statement before the continue has the result printed, and the continue statement (now on L131) is now covered
  • when inserting a pass statement on L130, I get cov fail on L131 (where the continue now is). pass might be special-cased though idk.
    • I get same behaviour if inserting "hello", or assert True on L130
  • it does not manifest on py310 or later
  • removing --cov-branch made no difference
  • I tried downgrading both pytest-cov and coverage, but going as far back as pytest-cov<4 and coverage<7 made no difference
  • unsurprisingly it's not pytest-cov, running pytest directly with coverage run -m pytest ... made no difference

[...]

docs/usage.rst Outdated
Comma-separated list of methods which should be used with :meth:`trio.Nursery.start`/:meth:`anyio.abc.TaskGroup.start` when opening a context manager,
in addition to the default :func:`trio.run_process`, :func:`trio.serve_tcp`, :func:`trio.serve_ssl_over_tcp`, and :func:`trio.serve_listeners`.
Comma-separated list of methods which should be used with :meth:`trio.Nursery.start`/:meth:`anyio.abc.TaskGroup.start` when opening a context manager.
This includes startable functions in the trio and anyio standard library by default, namely :func:`trio.run_process`, :func:`trio.serve_tcp`, :func:`trio.serve_ssl_over_tcp`, :func:`trio.serve_listeners`, :func:`trio.serve`, :func:`anyio.run_process`, :func:`anyio.serve_tcp`, :func:`anyio.serve_ssl_over_tcp`, :func:`anyio.serve_listeners`, and :func:`anyio.serve`.
Copy link
Member

Choose a reason for hiding this comment

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

I think that the terms 'standard library' and 'by default' can be confusing here; I'd say something like "We then add known functions from AnyIO and Trio to this list, specifically [...]."

docs/usage.rst:268: WARNING: py:func reference target not found: trio.serve
docs/usage.rst:268: WARNING: py:func reference target not found: anyio.serve_tcp
docs/usage.rst:268: WARNING: py:func reference target not found: anyio.serve_ssl_over_tcp
docs/usage.rst:268: WARNING: py:func reference target not found: anyio.serve_listeners
docs/usage.rst:268: WARNING: py:func reference target not found: anyio.serve
docs/usage.rst:271: WARNING: undefined label: 'async114'

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, turns out there is not a single function in anyio that takes a task_status parameter. The serve_xxx functions don't exist, and neither anyio.run_process has a task_status parameter nor any of the serve methods do. A search through the code and docs also reveal nothing. So ASYNC113 in fact does have several false alarms currently. Should change the code to also take into account the base of the attribute and not match on any xxx.run_process.

flake8_async/visitors/visitors.py Outdated Show resolved Hide resolved
Comment on lines 112 to 116
with trio.open_nursery(10) as s:
s.shield = myvar
await foo() # safe in theory, error: 12, Statement("try/finally", lineno-6)
# this is not safe in theory - because `trio.open_nursery` is an async cm,
# so it's not possible to open a nursery at all.
await foo() # error: 12, Statement("try/finally", lineno-8)
Copy link
Member

Choose a reason for hiding this comment

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

So this should be async with, yeah? Also nurseries only checkpoint on exit, so I think it is safe?

Copy link
Member Author

@jakkdl jakkdl May 30, 2024

Choose a reason for hiding this comment

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

oh, huh. https://trio.readthedocs.io/en/stable/reference-core.html#trio.open_nursery does say "It does not block on entry" (and I guess block==checkpoint) and I tried it just now to confirm:

import trio


async def foo():
    with trio.CancelScope() as cs:
        cs.cancel()
        async with trio.open_nursery() as nursery:
            print("a")
            nursery.cancel_scope.shield = True
            await trio.lowlevel.checkpoint()
            print("b")
        print("c")


trio.run(foo)

which prints all of a, b, and c.

So I guess we have double false alarms here, one because async with trio.open_nursery doesn't checkpoint, and one because we don't parse <nursery_name>.cancel_scope.[deadline/shield]

Copy link
Member Author

@jakkdl jakkdl Jun 4, 2024

Choose a reason for hiding this comment

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

wait, it doesn't block/checkpoint on exit either??

import trio

async def foo():
    with trio.CancelScope() as cs:
        cs.cancel()
        async with trio.open_nursery():
            print("in nursery")
        print("after nursery")

trio.run(foo)
$ python foo.py
in nursery
after nursery

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not block on entry; on exit it blocks until all child tasks have exited.

oh, hm. I guess that is meant to be read "if there are child tasks still alive, block until they have exited. If there are no child tasks, or they have all exited, it doesn't block."
Which in the end means that open_nursery never checkpoints on entry, and sometimes checkpoints on exit.

tox.ini Outdated Show resolved Hide resolved
docs/rules.rst Outdated Show resolved Hide resolved
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

LGTM overall; since this won't trigger a release let's deal with the false alarms in a follow-up 🙂

@Zac-HD Zac-HD merged commit 0748ce7 into python-trio:main May 30, 2024
10 checks passed
@jakkdl jakkdl deleted the async111_asyncio branch June 3, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants