Never start a queued task that is cancelled or stopped; two queues for the process backend by woutdenolf · Pull Request #508 · ewoks-kit/ewoksorange

woutdenolf · 2026-10-21T13:04:02Z

Closes #514

Stack (split from #501, merge in order):

Qt test fixes: drop test_owwidget_drop timing dependency, fix Windows crash at exit #506 Qt test fixes: drop test_owwidget_drop timing dependency, fix Windows crash at exit
Rename TaskFuture.abort/aborted to request_stop/stop_requested #507 Rename TaskFuture.abort/aborted to request_stop/stop_requested
Never start a queued task that is cancelled or stopped; two queues for the process backend #508 Never start a queued task that is cancelled or stopped; two queues for the process backend 👈 this PR
EwoksExecutor: completion signals after start signals, finished for every execution #509 EwoksExecutor: completion signals after start signals, finished for every execution
Execute subprocess tasks with pebble, add TaskFuture.abort and TaskFuture.kill #501 Execute subprocess tasks with pebble, add TaskFuture.abort and TaskFuture.kill
Doc: executor contract, backend diagrams and glossary #510 Doc: executor contract, backend diagrams and glossary

PR summary

Start gate: the first of cancel() / request_stop() that reaches a queued task prevents it from starting.
- cancel(): result() raises CancelledError.
- request_stop(): result() raises the new TaskStoppedError.
- Such a task never emits aboutToStart or started.
- Later calls on the same queued task return False. On a finished task they do nothing.
Process backend: the manager Events and the single events queue are replaced by two queues.
- Runner → controller: "about to start", "started", progress.
- Controller → runner: the start decision, then stop requests.
- The runner asks the controller before creating the task, so a cancel or stop that races with the worker picking up the task is honoured.
_progress.py → _messages.py. The endpoints are named controller and runner.
Still concurrent.futures.ProcessPoolExecutor (pebble comes later).

AI Disclosure

AI tools claude used for generate the code

🤖 Generated with Claude Code

… talk to the process runner through one queue per direction Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

codecov · 2026-10-21T13:12:04Z

Codecov Report

❌ Patch coverage is 84.03756% with 34 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...rc/ewoksorange/gui/concurrency/_runners/process.py	31.57%	26 Missing ⚠️
src/ewoksorange/gui/concurrency/_messages.py	85.71%	4 Missing ⚠️
...c/ewoksorange/gui/concurrency/_runners/abstract.py	89.65%	3 Missing ⚠️
src/ewoksorange/gui/concurrency/future.py	92.85%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

payno

Looks good but this was an hard one.
I didn't went deeply into the tests.

payno · 2026-10-22T08:29:17Z

+    def try_start(self) -> Optional[StartDenial]:
+        """Mark the task as started unless it was denied. Returns the denial."""
+        return self._gate.open()



This seems a bit weird because it uses _gate that is defined in parent class TaskController and there is no other operation using the gate or try_start in the children class.

In-process controller (sync, thread): the runner is in the same process, so it calls controller.try_start() directly.

Process controller: the runner is in another process, so it asks over the queue, and the controller calls self._gate.open() in _allow_start.

The process controller opens the gate too, in _allow_start, when the runner asks over the queue. Only in-process runners can call it directly, so try_start lives here.

payno · 2026-10-22T11:41:55Z

+        future.cancel()
+        return True


I would expects that we return the result of the future.cancel no ?

Suggested change

future.cancel()

return True

return future.cancel()

No: if a worker already picked up the task, future.cancel() returns False, but the gate already denied the start, so the task is cancelled anyway. future.cancel() only completes a still-queued future early.

payno · 2026-10-22T12:03:56Z

+
+
+class StartGate:
+    """Decides once whether a task may start."""


Suggested change

"""Decides once whether a task may start."""

"""Decides once whether a task may start.

The gate is shared by two sides, each calling one method:

- the runner, which executes the task, calls `open` right before creating

the task to ask whether it may start it.

- the caller, which controls the task, calls `deny` to prevent the task from

starting.

The first of the two to take the lock makes the decision.

"""

Added, shortened. In the process backend the controller calls open for the runner, so I wrote "the side executing the task" instead of "the runner".

payno · 2026-10-22T12:04:22Z

+        return self._denial
+
+    def deny(self, denial: StartDenial) -> bool:
+        """Prevent the task from starting.


Suggested change

"""Prevent the task from starting.

"""Prevent the task from starting. Called by the side controlling the task.

payno · 2026-10-22T12:05:21Z

+            return True
+
+    def open(self) -> Optional[StartDenial]:
+        """Let the task start unless it was denied. Returns the denial."""


Suggested change

"""Let the task start unless it was denied. Returns the denial."""

"""Let the task start unless it was denied. Called by the side executing

the task, right before creating it.

Returns the denial, or `None` when the task may start.

"""

payno · 2026-10-22T12:11:24Z

-        raise NotImplementedError
+        if self._gate.deny(StartDenial.STOPPED):
+            return True
+        if not self._gate.started:


Was not that direct to me. Nevertheless the comment might be over-killed. I let you judge.

Suggested change

if not self._gate.started:

# case task already started or already denied

if not self._gate.started:

Added, placed on the return False: past deny(), not started means denied before.

payno · 2026-10-22T12:15:08Z

        raise NotImplementedError

+    def stop_requested(self) -> bool:
+        """Return True if the stop request prevented the start or reached the task."""


Suggested change

"""Return True if the stop request prevented the start or reached the task."""

"""Return True if the stop request prevented the start or reached the task."""

# Stopped before starting: the gate holds the denial, nothing was sent (set self._gate.denial).

# Stopped while running: the stop request was sent to the task (set self._stop_sent).

Added, without the parentheses.

payno · 2026-10-22T12:25:45Z

        cancelled). Manager queues are FIFO, so draining up to the sentinel
        delivers all of them.
        """
        if self._relay_thread is None:


Not related to the PR but re-reading the classes this was not obvious

Suggested change

if self._relay_thread is None:

# Guard in case never watched. Should never happen.

if self._relay_thread is None:

payno · 2026-10-22T12:33:53Z

+    name = task_class.__name__
+    if denial is StartDenial.STOPPED:
+        return TaskStoppedError(f"Task {name} was stopped before it started")
+    raise ValueError(f"Start denial {denial} is not handled")


raise is done from raise _denial_error(self._task_class, denial). So from this and the docstring I would expect more something like:

Suggested change

raise ValueError(f"Start denial {denial} is not handled")

return ValueError(f"Start denial {denial} is not handled")

payno · 2026-10-22T12:51:08Z

+        def _read_to_runner_queue():
+            try:
+                while True:
+                    message = self._to_runner_queue.get()
+                    if message is ToRunner.REQUEST_STOP:
+                        stop_event.set()
+                    elif message is ToRunner.END:
+                        return
+            except self._TRANSIENT_ERRORS:
+                pass


nested function + a try + a while starts to get difficult to read. I would remove at least one.

Suggested change

def _read_to_runner_queue():

try:

while True:

message = self._to_runner_queue.get()

if message is ToRunner.REQUEST_STOP:

stop_event.set()

elif message is ToRunner.END:

return

except self._TRANSIENT_ERRORS:

pass

def _relay_stop_requests() -> None:

"""Set `stop_event` on stop requests until `ToRunner.END`."""

while True:

message = self._to_runner_queue.get()

if message is ToRunner.REQUEST_STOP:

stop_event.set()

elif message is ToRunner.END:

return

def _read_to_runner_queue() -> None:

"""Set `stop_event` on stop requests until `ToRunner.END` or the manager shuts down."""

try:

_relay_stop_requests()

except self._TRANSIENT_ERRORS:

pass

Maybe the function could also be moved to private at the class scope. No strong opinion there.

Moved to a private method instead of splitting it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…turns False Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…er never watched Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…sing it Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Never start a queued task that is cancelled or requested to stop, and…

c7957bd

… talk to the process runner through one queue per direction Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

woutdenolf changed the base branch from request_stop_rename to main October 22, 2026 08:41

payno approved these changes Oct 22, 2026

View reviewed changes

woutdenolf mentioned this pull request Oct 24, 2026

Race condition with InterruptStopTask #514

Closed

StartGate: document which side calls open and deny

ec30e17

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

woutdenolf force-pushed the process_start_gate branch from e14a18a to ec30e17 Compare October 24, 2026 08:01

woutdenolf and others added 5 commits October 24, 2026 10:03

TaskController.request_stop: comment why a task that did not start re…

1a82ead

…turns False Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

TaskController.stop_requested: comment why both conditions are needed

a609c3b

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

ProcessTaskController.stop_watching: comment the guard for a controll…

5998830

…er never watched Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

_denial_error: return the error of an unhandled denial instead of rai…

e2f4754

…sing it Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

ProcessTaskRunner: move the stop request reader to a private method

84a8985

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

woutdenolf enabled auto-merge October 24, 2026 08:17

woutdenolf merged commit 67b2926 into main Oct 24, 2026
15 checks passed



		class StartGate:
		"""Decides once whether a task may start."""

-    """Decides once whether a task may start."""
+    """Decides once whether a task may start.
+    The gate is shared by two sides, each calling one method:
+    - the runner, which executes the task, calls `open` right before creating
+      the task to ask whether it may start it.
+    - the caller, which controls the task, calls `deny` to prevent the task from
+      starting.
+    The first of the two to take the lock makes the decision.
+    """

	"""Prevent the task from starting.
	"""Prevent the task from starting. Called by the side controlling the task.

-        """Let the task start unless it was denied. Returns the denial."""
+        """Let the task start unless it was denied. Called by the side executing
+        the task, right before creating it.
+        Returns the denial, or `None` when the task may start.
+        """

	if not self._gate.started:
	# case task already started or already denied
	if not self._gate.started:

	if self._relay_thread is None:
	# Guard in case never watched. Should never happen.
	if self._relay_thread is None:

	raise ValueError(f"Start denial {denial} is not handled")
	return ValueError(f"Start denial {denial} is not handled")

Conversation

woutdenolf commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR summary

AI Disclosure

Uh oh!

codecov Bot commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

payno left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

woutdenolf commented Oct 21, 2026 •

edited

Loading

codecov Bot commented Oct 21, 2026 •

edited

Loading