[PULP-218] Add timeout to immediate tasks #6155
Conversation
pedro-psb
changed the title
pulpcore/tasking/tasks.py
Outdated
| signal.signal(signal.SIGALRM, timeout_handler) | ||
| signal.alarm(IMMEDIATE_TASK_TIMEOUT) |
There was a problem hiding this comment.
Oh, interesting.
This is quite close to low level process handling. I wonder if this might interfere with other uses of the signal handler.
Do you think we can instead require that all immediate tasks are async functions? We could then implement the timeout on the asyncio level with much more confidence.
There was a problem hiding this comment.
Do you think we can instead require that all immediate tasks are async functions? We could then implement the timeout on the asyncio level with much more confidence.
Yes. I dont have a good idea of how much friction this imposes, but that's definitely more safe.
There was a problem hiding this comment.
@mdellweg Hey! Can you provide an early review for the api worker part?
pedro-psb
force-pushed
the
add-timeout-to-immediate-task
branch
from
6b4c39f to
138be99
Compare
pedro-psb
force-pushed
the
add-timeout-to-immediate-task
branch
from
138be99 to
08a9c14
Compare
pedro-psb
force-pushed
the
add-timeout-to-immediate-task
branch
from
7eb645f to
c151ecf
Compare
pedro-psb
force-pushed
the
add-timeout-to-immediate-task
branch
3 times, most recently
from
54c86f9 to
1704eb4
Compare
pedro-psb
force-pushed
the
add-timeout-to-immediate-task
branch
from
1704eb4 to
88fc96d
Compare
pulpcore/tasking/tasks.py
Outdated
| loop.run_until_complete(result) | ||
| if immediate: | ||
| try: | ||
| loop.run_until_complete(asyncio.wait_for(result, timeout=IMMEDIATE_TIMEOUT)) |
There was a problem hiding this comment.
Looks like a good first cut. But we still stop the heartbeat loop for up to 5 seconds here.
We should definitely plan on rewriting the workers in async.
There was a problem hiding this comment.
We should definitely plan on rewriting the workers in async.
Would you say that's a blocker (for running on foreground)?
I was trying to find a way to integrate the coroutine control yielding with the select-loop, but I'm reaching dark corners.
If that's a blocker this PR could be really about the timeout (as originally intented). The commit "wip: add timeout to supervise immediate tasks" adds a timeout to the regular task process. Maybe we could stop there.
There was a problem hiding this comment.
Not necessarily. It's a certain risk. But it's nothing we cannot revert when things go bad.
There was a problem hiding this comment.
Maybe after all it's wise to split the work into smaller chunks to reduce that risk.
Ensuring that immediate tasks don't run too long is still an incremental improvement.
pedro-psb
force-pushed
the
add-timeout-to-immediate-task
branch
from
6b2781e to
1b427eb
Compare
|
We cannot require all immediate tasks to be async without properly deprecating it for plugins. |
pedro-psb
force-pushed
the
add-timeout-to-immediate-task
branch
from
00a071e to
e834821
Compare
|
No idea why this fails only on s3: An exception does get caught... |
No description provided.