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

Investigate test failures #2

Closed
thedrow opened this issue Oct 4, 2020 · 15 comments
Closed

Investigate test failures #2

thedrow opened this issue Oct 4, 2020 · 15 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@thedrow
Copy link
Collaborator

thedrow commented Oct 4, 2020

Both the test_multiple_models and the test_queued tests are failing, even with asyncio as the anyio backend.
We should investigate why that happens and fix it as soon as possible.

@thedrow thedrow added bug Something isn't working help wanted Extra attention is needed good first issue Good for newcomers labels Oct 4, 2020
@thedrow thedrow self-assigned this Oct 4, 2020
@thedrow
Copy link
Collaborator Author

thedrow commented Oct 12, 2020

@aleneum If you don't mind, please take a look.

@aleneum
Copy link
Member

aleneum commented Oct 13, 2020

It seems like cancel_soon isn't actually cancelled when the scope is.

import anyio
from anyio import create_task_group, get_cancelled_exc_class, open_cancel_scope


async def cancel_soon():
    await anyio.sleep(1)
    raise TimeoutError("Callback was not cancelled!")


async def call_delayed(func, time):
    await anyio.sleep(time)
    await func()


async def main():
    async with create_task_group() as tg:
        async with open_cancel_scope(shield=False) as scope:
            await tg.spawn(cancel_soon)
            await tg.spawn(call_delayed, scope.cancel, 0.1)

anyio.run(main)

@aleneum
Copy link
Member

aleneum commented Oct 13, 2020

Okay, so open_cancel_scope must prepend create_task_group or the cancel scope of tg has to be directly used:

works:

async def main():
    async with open_cancel_scope(shield=False) as scope:
        async with create_task_group() as tg:
            await tg.spawn(cancel_soon)
            await tg.spawn(call_delayed, scope.cancel, 0.1)

also works:

async def main():
    async with create_task_group() as tg:
        await tg.spawn(cancel_soon)
        await tg.spawn(call_delayed, tg.cancel_scope.cancel, 0.1)

@aleneum
Copy link
Member

aleneum commented Oct 13, 2020

If cancel_soon is rewritten into:

async def cancel_soon():
    try:
        await anyio.sleep(1)
        raise TimeoutError("Callback was not cancelled!")
    except get_cancelled_exc_class():
        print("Cancel soon was cancelled")
        raise

the cancellation messages is shown but the TimeoutError is raised anyway 😕

@aleneum
Copy link
Member

aleneum commented Oct 13, 2020

I guess some of the weirdness is due to the fact that CancelScope.cancel needs to be awaited while an asyncio.Task can be cancelled synchronously. However, even if I await scope.cancel, things dont work. I added some print to switch_model_context and process_context and ran test_multiple_models:

current scope is <anyio._backends._asyncio.CancelScope object at 0x7f67efff53c0>
cancel scope <anyio._backends._asyncio.CancelScope object at 0x7f67f012bbc0>
scope <anyio._backends._asyncio.CancelScope object at 0x7f67efff53c0> was cancelled

transitions attempts to cancel the correct scope (0x7f67f012bbc0) but the scope that exits is the CURRENT scope (0x7f67efff53c0). Shielding does not help and reraising the cancel event does not help either.

changes:

    async def process_context(self, func, model):
        if self.current_context.get() is None:
            try:
                async with open_cancel_scope(shield=False) as scope:
                    self.current_context.set(scope)
                    return await func()
            except get_cancelled_exc_class():
                print(f"scope {scope} was cancelled")
                return False
        return await func()

    async def switch_model_context(self, model):
        current_scope = self.current_context.get()
        running_scope = self.async_tasks.get(model, None)
        if current_scope != running_scope:
            self.async_tasks[model] = self.current_context.get()
            if running_scope is not None:
                print(f"current scope is {current_scope}")
                print(f"cancel scope {running_scope}")
                await running_scope.cancel()

@thedrow
Copy link
Collaborator Author

thedrow commented Oct 13, 2020

So how do we get the correct scope?

@aleneum
Copy link
Member

aleneum commented Oct 13, 2020

it seems that open_cancel_scope does not raise an exception when its cancelled. I dont know enough about anyio internals to get why this is the case. This can be fixed quite easily as seen in d1903c3. However (and that took waaaay longer that it should), it seems that depending on the backend tasks might not been cancelled appropriately. The 'fix' above works for asyncio and trio but not curio

@aleneum
Copy link
Member

aleneum commented Oct 13, 2020

for queing to work. tasks need to be passed to (Async)Machine._process. That has been fixed as well.

@aleneum
Copy link
Member

aleneum commented Oct 13, 2020

oh, remark. for this to work AsyncMachine has to be tweaked. switch_model_context needs to be awaited.

@aleneum
Copy link
Member

aleneum commented Oct 13, 2020

I applied that change to transitions master (See here).

@aleneum
Copy link
Member

aleneum commented Oct 13, 2020

I added the changes that were made in transitions to deal with more complex task configurations (protected_tasks) and clean up after a trigger has been processed to prevent memory leaks when many (as in thousands) models are used.

@aleneum
Copy link
Member

aleneum commented Oct 13, 2020

So, for curio the 'problem' is, that spawned tasks seem to run in the same context.

@aleneum
Copy link
Member

aleneum commented Oct 13, 2020

In acbcdae I added special treatment for curio CancelScopes. I also added a test to check whether this treatment might cause callbacks to cancel themselves when they transititon to another state. It passes for now.

@thedrow
Copy link
Collaborator Author

thedrow commented Oct 13, 2020

Wonderful!
I'll wait for a new release before releasing this library.
Please ping me when you do so.

@thedrow thedrow closed this as completed Oct 13, 2020
@aleneum
Copy link
Member

aleneum commented Nov 12, 2020

just fyi: 0.8.5 has been released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants