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

Remove all uses of asyncio.shield() in our codebase #359

Closed
achimnol opened this issue Feb 3, 2022 · 1 comment
Closed

Remove all uses of asyncio.shield() in our codebase #359

achimnol opened this issue Feb 3, 2022 · 1 comment
Assignees
Labels
comp:agent Related to Agent component comp:common Related to Common component comp:manager Related to Manager component type:refactor Refactor codes or add tests.
Milestone

Comments

@achimnol
Copy link
Member

achimnol commented Feb 3, 2022

It may make dangling but running tasks when shutting down, because it only cancels the outer await but keeps the inner task intact. asyncio.shield() is useful to protect tasks that are "not well written to react on cancellation" but requires a strong assumption that all shielded tasks should terminate within a finite, short period of time.

Historically aiopg did not handle cancellation of transactions during entering of them (aio-libs/aiopg#332), and we have experienced exhaustion of database connections due to accumulation of unreleased transactions and connections when there are cloud users from unstable WiFi (aiohttp cancels request handler tasks when the connection is interrupted). This is where we started to use asyncio.shield() for some database-critical operations, such as AgentRegistry.enqueue_session() called from API handlers. Now we don't use aiopg with SQLAlchemy but asyncpg, which handles this better.

Let's remove uses of asyncio.shield() and replace it with aiotools.PersistentTaskGroup if necessary.
Let's make separate PRs for each module code update.

I expect that this would reduce the possibility of getting stuck while shutting down services.

@achimnol achimnol added comp:manager Related to Manager component comp:agent Related to Agent component comp:common Related to Common component type:refactor Refactor codes or add tests. labels Feb 3, 2022
@achimnol achimnol added this to the 21.03 milestone Feb 3, 2022
@achimnol achimnol modified the milestones: 21.03, 21.09 Feb 4, 2022
@achimnol
Copy link
Member Author

For the asyncio.shield() usescases of short-lived database transaction wrappers in the api.stream module, let's wrap the ptaskgroup.create_task() with asyncio.shield() and apply the shutdown timeout option of ptaskgroup (though achimnol/aiotools#33 must be implemented first) so that database transactions always complete in common scenarios. For now, just wrap them with a ptaskgroup (which is stored in the app-local context).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:agent Related to Agent component comp:common Related to Common component comp:manager Related to Manager component type:refactor Refactor codes or add tests.
Projects
None yet
Development

No branches or pull requests

2 participants