[PULP-467] Re-add timeout to immediate task with regression fix #6463

pedro-psb · 2025-04-08T20:02:22Z

Experiment with creating async version of dispatch to make async timeout work from the content app.

pedro-psb · 2025-04-09T21:19:49Z

I'm hitting a wall here on implementing an async dispatch. Not sure its a good ideal at all.
Django does not support async transaction, and making the transaction run in a 'sync_to_async thread' is messing with locks management.

The image below illustrates how I perceive the issue.

If we didnt use ExitStack, we could do some hacking to handle executing the cleanup code in both sync/async context, but I'm not confident in doing the proper error handling there.

pedro-psb · 2025-04-09T21:24:47Z

I was thinking in another approach to avoid having to implement async dispatch, at least for now.
Instead of calling dispatch directly, the content-handler could call the dispatch via the API.
This sounds more stable overall (because we have a more predictable running context for dispatch) and would solve the pull-through regression.

But I wonder if that wouldn't add too much overhead to the pull-through content serving branch.

mdellweg · 2025-04-15T08:00:13Z

pulpcore/app/models/repository.py

-        return dispatch(add_and_remove, kwargs=body, exclusive_resources=[self], immediate=True)
+        task = dispatch(add_and_remove, kwargs=body, exclusive_resources=[self], immediate=True)
+        # TODO: check if task completed succesfully, as its run in the worker now
+        return task


Why do we return anything at all? Should this not be a dispatch and forget action?

Yeah, maybe it should.
I assumed that the request would fail when the task failed because it is immediate, but actually it may fail or silently ignore, as it may get defered.

The "request" is not interested in this task. It already got the artifact delivered and is happy at this point.

mdellweg · 2025-04-16T18:57:08Z

pulpcore/tasking/tasks.py

+    thread_name = threading.current_thread().name
+    # ThreadPoolExecutor names threads like: "ThreadPoolExecutor-0_0"
+    return "ThreadPoolExecutor" in thread_name


I wish there was a better way, not using magic values.

Another approach is to use some context variables.
I think we could use threading.local() or something like that.
https://docs.python.org/3/library/threading.html#thread-local-data

Or even context variables. I'm not sure the best fit.
Do you prefer that we use something like that?

Hmm. I don't know. But after all, we want to be able to schedule the async task back to the main thread loop (once we figure out how two reliably...). So we should not over engineer this stop gap here.

That's a good reminder.
I've quickly tried contextvars. It passed the manual test locally but failed CI, so I reverted it.
The other requested changes are in place.

We should make this comment much more scary. We really want to improve this part ASAP.

mdellweg · 2025-04-16T18:58:31Z

pulpcore/tasking/tasks.py

@@ -174,6 +181,11 @@ def dispatch(

    assert deferred or immediate, "A task must be at least `deferred` or `immediate`."

+    # Can't run short tasks immediately if running from thread pool
+    force_defer = True if running_from_thread_pool() else False


True if True else False? This is not wrong, but way too complicated...

Where's the suggest feature when you need it...

I thought it reads better, but not big deal.

Well your function returns a boolean already. And you translate a boolean into the same boolean using the ternary operator. This is very verbose for nothing. Sorry.

mdellweg · 2025-04-16T19:00:19Z

pulpcore/tasking/tasks.py

+    # Can't run short tasks immediately if running from thread pool
+    force_defer = True if running_from_thread_pool() else False
+    short = immediate
+    immediate = short and not force_defer


What this all means is
immediate &= not running_in_thread_pool()

Also we should move the assert from above below this or we might produce improper tasks again.

Yes, I was trying to enforce some semantics around that.

Also we should move the assert from above below this or we might produce improper tasks again.

Good point

pedro-psb · 2025-04-16T20:48:06Z

FWIW, I'm testing this manually with the script in #6429 in an environment with npm plugin installed.

mdellweg

Can you rebase this work?

mdellweg · 2025-04-24T14:45:55Z

CHANGES/plugin_api/+revert-immediate-task-timeout.bugfix

Has this revert not been released?
Then we should not add any changelog at all for this.

Actually, this file should not be there anymore at this point in time.

That has been released. I would also think that this should't be here.

It was just outdated

mdellweg · 2025-04-24T14:50:41Z

pulpcore/app/tasks/test.py

@@ -14,6 +14,13 @@ def sleep(interval):
    sleep(interval)


+async def asleep(interval):
+    """Async function that sleeps."""
+    import asyncio


The way we do imports in this file is unfortunate. Would you mind cleaning it up?

Let's do "import time" at the top, e.g..

This reverts commit 2b3c88a.

mdellweg · 2025-04-25T08:39:25Z

pulpcore/app/tasks/base.py

@@ -111,3 +113,29 @@ def general_multi_delete(instance_ids):
    with transaction.atomic():
        for instance in instances:
            instance.delete()
+
+
+async def ageneral_update(instance_id, app_label, serializer_name, *args, **kwargs):


Sorry, I keep coming up with more and more stuff...
But i think we should deprecate the old tasks for deletion in pulp 4.

Adding a deprecation message makes the CI unhappy.
Not sure how to handle that.

mdellweg · 2025-04-25T08:40:32Z

pulpcore/app/tasks/test.py

@@ -28,5 +31,5 @@ def dummy_group_task(inbetween=3, intervals=None):
    task_group = TaskGroup.current()
    for interval in intervals:
        dispatch(sleep, args=(interval,), task_group=task_group)
-        sleep(inbetween)
+        time.sleep(inbetween)


Now we are calling the proper function here. Thank you!

mdellweg · 2025-04-25T08:42:48Z

pulpcore/tasking/tasks.py

+    thread_name = threading.current_thread().name
+    # ThreadPoolExecutor names threads like: "ThreadPoolExecutor-0_0"
+    return "ThreadPoolExecutor" in thread_name


We should make this comment much more scary. We really want to improve this part ASAP.

Immediate/short tasks are now run inside an event loop, but that doesnt work inside a thread from a thread pool, like it happens with on the pull-through workflow from the content-app. This enforces that the short task can't run immediately if the dispatch is done from inside such a context. Closes: pulp#6429

mdellweg · 2025-04-26T08:48:48Z

pulpcore/tasking/tasks.py

+def running_from_thread_pool() -> bool:
+    # TODO: this needs an alternative approach ASAP!
+    # Currently we rely on the weak fact that ThreadPoolExecutor names threads like:
+    # "ThreadPoolExecutor-0_0"


github-actions bot added multi-commit no-changelog no-issue labels Apr 8, 2025

mdellweg reviewed Apr 15, 2025

View reviewed changes

pedro-psb force-pushed the add-timeout-to-immeidate-task-2 branch 2 times, most recently from 967eae6 to ca4c621 Compare April 16, 2025 17:40

github-actions bot removed the no-changelog label Apr 16, 2025

pedro-psb changed the title ~~Add timeout to immediate task (with regression fix)~~ [PULP-467] Add timeout to immediate task (with regression fix) Apr 16, 2025

pedro-psb changed the title ~~[PULP-467] Add timeout to immediate task (with regression fix)~~ [PULP-467] Re-add timeout to immediate task with regression fix Apr 16, 2025

pedro-psb marked this pull request as ready for review April 16, 2025 17:47

mdellweg requested changes Apr 16, 2025

View reviewed changes

github-actions bot added the no-changelog label Apr 16, 2025

pedro-psb force-pushed the add-timeout-to-immeidate-task-2 branch 3 times, most recently from 0ee08f1 to 4e522da Compare April 16, 2025 20:35

pedro-psb force-pushed the add-timeout-to-immeidate-task-2 branch from 4e522da to 2c3a969 Compare April 17, 2025 12:23

mdellweg reviewed Apr 24, 2025

View reviewed changes

Reapply "Add timeout to immediate task execution"

5f6f916

This reverts commit 2b3c88a.

pedro-psb force-pushed the add-timeout-to-immeidate-task-2 branch from 2c3a969 to d004162 Compare April 24, 2025 17:40

mdellweg reviewed Apr 25, 2025

View reviewed changes

pedro-psb force-pushed the add-timeout-to-immeidate-task-2 branch from d004162 to db95127 Compare April 25, 2025 13:51

pedro-psb force-pushed the add-timeout-to-immeidate-task-2 branch from db95127 to 8dcbc8c Compare April 25, 2025 15:07

mdellweg approved these changes Apr 26, 2025

View reviewed changes

mdellweg merged commit 6eda2af into pulp:main Apr 26, 2025
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PULP-467] Re-add timeout to immediate task with regression fix #6463

[PULP-467] Re-add timeout to immediate task with regression fix #6463

pedro-psb commented Apr 8, 2025

pedro-psb commented Apr 9, 2025

pedro-psb commented Apr 9, 2025

mdellweg Apr 15, 2025

pedro-psb Apr 16, 2025

mdellweg Apr 16, 2025

mdellweg Apr 16, 2025

pedro-psb Apr 16, 2025

pedro-psb Apr 16, 2025 •

edited

Loading

mdellweg Apr 16, 2025

pedro-psb Apr 16, 2025

mdellweg Apr 25, 2025

mdellweg Apr 16, 2025

pedro-psb Apr 16, 2025

mdellweg Apr 16, 2025

pedro-psb Apr 16, 2025

mdellweg Apr 16, 2025

pedro-psb Apr 16, 2025

pedro-psb Apr 16, 2025

pedro-psb commented Apr 16, 2025

mdellweg left a comment

mdellweg Apr 24, 2025

mdellweg Apr 24, 2025

pedro-psb Apr 24, 2025

pedro-psb Apr 24, 2025

mdellweg Apr 24, 2025

mdellweg Apr 25, 2025

pedro-psb Apr 25, 2025

mdellweg Apr 25, 2025

mdellweg Apr 25, 2025

mdellweg Apr 26, 2025

[PULP-467] Re-add timeout to immediate task with regression fix #6463

[PULP-467] Re-add timeout to immediate task with regression fix #6463

Conversation

pedro-psb commented Apr 8, 2025

pedro-psb commented Apr 9, 2025

pedro-psb commented Apr 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedro-psb Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedro-psb commented Apr 16, 2025

mdellweg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedro-psb Apr 16, 2025 •

edited

Loading