[RED-13] [ListenerMiddleware][Breaking] - Improve `listernerApi.fork` scheduling logic

[FaberVitale]

Description

Current implementation of listernerApi.fork has an implicit contract:

1. The output forked task is scheduled **synchronously** in a microtask.
2. The output forked task will **always** run if it is not cancelled **synchronously**.

The second clause can be problematic in certain situations because It is possible to cancel a forked task that we're going to execute anyway:

---

Examples

1. using process.nextTick

listenerMiddleware.startListening({
  actionCreator: increment,
  async effect(action, listenerApi) {
    const task = listenerApi.fork(async (forkApi) => { console.log(2); })

    globalThis?.process?.nextTick(() => { task.cancel(); console.log(1); })  // nextTick (node only) runs before the microtask queue
  },
})

2. using task.cancel in a scheduled microtask

listenerMiddleware.startListening({
  actionCreator: increment,
  async effect(action, listenerApi) {
   let task: ForkedTask<void>;
   
   globalThis?.queueMicrotask(() => { task?.cancel(); console.log(1); }) 

   task = listenerApi.fork(async (forkApi) => { console.log(2); })
  },
})

While these examples might seem contrived, we do not generally use node-only primitives in universal code, it highlights an issue with the current implementation that might cause problems if the user does not use forkApi.signal or the other forkApi methods.

---

Suggested changes

Change the listernerApi.fork contract to:

1. The output forked task is scheduled **synchronously** in a microtask.
- 2. The output forked task will **always** run if it is not cancelled **synchronously**.
+ 2. The output forked task will **always** run if it is not cancelled before the scheduled execution.

_RED-13

[markerikson]

Heh, I'm afraid this one is mostly going over my head :)

What's the specific concern here, and what's the actual proposed code change?

[FaberVitale]

What's the specific concern here

We currently execute tasks that might already be cancelled. It can be a problem if the user doesn't check forkApi.signal or other forkApi methods that halt the task execution.

Consider the following example

  async effect(action, listenerApi) {
   let task: ForkedTask<void>;
   
   globalThis?.queueMicrotask(() => {
      if(checkACondition()) { task.cancel(); }
    }) 

   task = listenerApi.fork((forkApi) => { runImportantBusinessLogic() })
  }

Should we execute the forked task in this case? We can debate that we should check forkApi.signal before running runImportantBusinessLogic but it is counterintuitive that we are executing a task that has already been cancelled.

---

what's the actual proposed code change

We need to add a check after await Promise.resolve() and change the signature of runTask

/**
 * Runs a task and returns promise that resolves to {@link TaskResult}.
 * Second argument is an optional `cleanUp` function that always runs after task.
 *
 * **Note:** `runTask` runs the executor in the next microtask.
 * @returns
 */
export const runTask = async <T>(
  task: () => Promise<T>,
+ signal: AbortSignal,
  cleanUp?: () => void
): Promise<TaskResult<T>> => {
  try {
    await Promise.resolve()
+   validateActive(signal)
    const value = await task()
    return {
      status: 'ok',
      value,
    }
  } catch (error: any) {
    return {
      status: error instanceof TaskAbortError ? 'cancelled' : 'rejected',
      error,
    }
  } finally {
    cleanUp?.()
  }
}

[markerikson]

Looking back at this. What's actually "breaking" here? Seems like runTask is internal? If you're referring to the tweak in timing behavior, I don't see that as being actually "breaking" - we haven't formally documented that anywhere.

[FaberVitale]

Sorry for the late reply,

I don't see that as being actually "breaking" - we haven't formally documented that anywhere.

That's right, I'm just afraid that code that relied implicitly on this behavior could not work as designed with this change.

Anyway, I still believe that this change should be added to the listenerMiddleware in either a minor or major release.

[markerikson]

@FaberVitale this one's kind of low priority atm. Are you up for submitting a PR with the changes you have in mind?

[FaberVitale]

@FaberVitale this one's kind of low priority atm. Are you up for submitting a PR with the changes you have in mind?

I will do it 👍

[FaberVitale]

I can no longer repro this issue reported in 2022.