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.

Sign up for GitHub

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

ext: fix issues in net gc task #7904

Merged
merged 4 commits into from
Oct 23, 2024
Merged

ext: fix issues in net gc task #7904

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1dd55f8
ext: fix issues in net gc task
fantix Oct 22, 2024
b096aae
log deleted count instead of all IDs
fantix Oct 22, 2024
f4f522d
crf: check is_database_connectable() more accurately
fantix Oct 22, 2024
8f0fd9a
Merge remote-tracking branch 'origin/master' into fix-net-gc
fantix Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions edb/server/net_worker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ def _warn(e):
)
async for iteration in rloop:
async with iteration:
result = await execute.parse_execute_json(
result_json = await execute.parse_execute_json(
db,
"""
with requests := (
Expand All @@ -351,6 +351,7 @@ def _warn(e):
variables={"expires_in": expires_in.to_backend_str()},
cached_globally=True,
)
result: list[dict] = json.loads(result_json)
if len(result) > 0:
logger.info(f"Deleted requests: {result!r}")
else:
Expand All @@ -363,6 +364,10 @@ async def _gc(tenant: edbtenant.Tenant, expires_in: statypes.Duration) -> None:
for db in tenant.iter_dbs():
if db.name == defines.EDGEDB_SYSTEM_DB:
continue
if not tenant.is_database_connectable(db.name):
# Don't run the net_worker if the database is not
# connectable, e.g. being dropped
continue
g.create_task(_delete_requests(db, expires_in))
except Exception as ex:
logger.debug(
Expand All @@ -382,7 +387,8 @@ async def gc(server: edbserver.BaseServer) -> None:
if tenant.accept_new_tasks
]
try:
await asyncio.wait(tasks)
if tasks:
await asyncio.wait(tasks)
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we using a TaskGroup here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I guess we can; this was a carried-over from an old commit, let me try again so that we know for sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah it's just unnecessary, because the Task we're in here is safe without cancellation - the server doesn't need to wait for gc() itself to clean up anything, while all child Tasks are already managed by the TaskGroup in each Tenant (by tenant.create_task()).

We would need an extra coroutine wrapper to use TaskGroup here:

async def _await_task(task: asyncio.Task):
    return await task


async def gc(server: edbserver.BaseServer) -> None:
    while True:
        try:
            async with asyncio.TaskGroup() as g:
                for tenant in server.iter_tenants():
                    if tenant.accept_new_tasks:
                        task = tenant.create_task(
                            _gc(tenant, NET_HTTP_REQUEST_TTL),
                            interruptable=False,
                        )
                        g.create_task(_await_task(task))
...

except Exception as ex:
logger.debug(
"GC of std::net::http::ScheduledRequest failed", exc_info=ex
Expand Down
Loading