feat: Add immediate:boolean to RunForkOptions

[TylorS]

Type

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

• Adds immediate: boolean to RunForkOptions to enable creating a Fiber which uses fiberRuntime.start() or fiberRuntime.resume()

• Adds RunCallbackOptions<E, A> which extends RunForkOptions with an onExit callback, such that runCallback has access to the same fork options as runFork.

Related

• Related Issue #
• Closes #

[changeset-bot]

🦋 Changeset detected

Latest commit: 426bdbb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 17 packages

Name	Type
effect	Minor
@effect/cli	Major
@effect/experimental	Major
@effect/opentelemetry	Major
@effect/platform-browser	Major
@effect/platform-bun	Major
@effect/platform-node	Major
@effect/platform	Major
@effect/printer-ansi	Major
@effect/printer	Major
@effect/rpc-http-node	Major
@effect/rpc-http	Major
@effect/rpc-nextjs	Major
@effect/rpc-workers	Major
@effect/rpc	Major
@effect/schema	Major
@effect/typeclass	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[mattiamanzati]

Bonus, could we please have this also on unsafeRunCallback? It is useful to have the callback eventually be called synchronously if the effect is sync as well

[TylorS]

@mattiamanzati I should be able to add the immediate flag there as well. I'm going to remove the Scope option though, as it could lead to memory leaks if the surrounding scope is open indefinitely as the Fiber is never removed from the Scope upon exiting

[TylorS]

@mattiamanzati Updated in the latest pushes

[mattiamanzati]

@TylorS Thanks 🙏
Can't wait for this PR to be accepted and released!

[mikearnaldi]

@mattiamanzati I should be able to add the immediate flag there as well. I'm going to remove the Scope option though, as it could lead to memory leaks if the surrounding scope is open indefinitely as the Fiber is never removed from the Scope upon exiting

Why would the fiber not being removed upon exit? that should be the behaviour

[TylorS]

@mikearnaldi When you do Scope.addFinalizer(scope, Fiber.interrupt(fiber)), there is no corresponding API for removing the finalizer from the Scope after the Fiber exits. So if the scope it is being added to lives indefinitely (i.e. the scope of a React component's event listeners), the Fibers will be unable to be garbage collected due to being kept within the open Scope.

[mikearnaldi]

@mikearnaldi When you do Scope.addFinalizer(scope, Fiber.interrupt(fiber)), there is no corresponding API for removing the finalizer from the Scope after the Fiber exits. So if the scope it is being added to lives indefinitely (i.e. the scope of a React component's event listeners), the Fibers will be unable to be garbage collected due to being kept within the open Scope.

you're right, I will investigate if adding an api to remove a finaliser may be useful

[TylorS]

In the same vein, I'm not positive that forkIn or forkScoped are entirely safe. They do utilize forked Scope's which allows the Fiber to be garbage collected after completion, but there's a (albeit smaller) memory leak of child scopes if the parent scope is kept open indefinitely.

[mikearnaldi]

/** @internal */
export const forkIn = dual<
  (scope: Scope.Scope) => <R, E, A>(self: Effect.Effect<R, E, A>) => Effect.Effect<R, never, Fiber.RuntimeFiber<E, A>>,
  <R, E, A>(self: Effect.Effect<R, E, A>, scope: Scope.Scope) => Effect.Effect<R, never, Fiber.RuntimeFiber<E, A>>
>(
  2,
  (self, scope) =>
    core.uninterruptibleMask((restore) =>
      core.flatMap(scope.fork(executionStrategy.sequential), (child) =>
        pipe(
          restore(self),
          core.onExit((exit) => child.close(exit)),
          fiberRuntime.forkDaemon,
          core.tap((fiber) =>
            child.addFinalizer(() =>
              core.fiberIdWith((fiberId) =>
                Equal.equals(fiberId, fiber.id()) ?
                  core.unit :
                  core.asUnit(core.interruptFiber(fiber))
              )
            )
          )
        ))
    )
)

This is safe, it forks a separate scope and that scope is closed as the fiber ends

[TylorS]

But closing that child/forked Scope doesn't remove its reference as a Finalizer in the parent Scope when it is closed

[TylorS]

I take that back, I found the code that keeps it safe here - https://github.com/Effect-TS/effect/blob/main/packages/effect/src/internal/fiberRuntime.ts#L2962

[tim-smart]

Bonus, could we please have this also on unsafeRunCallback? It is useful to have the callback eventually be called synchronously if the effect is sync as well

The current behaviour is to start immediately, so you will see no change with this PR.

[mattiamanzati]

Bonus, could we please have this also on unsafeRunCallback? It is useful to have the callback eventually be called synchronously if the effect is sync as well

The current behaviour is to start immediately, so you will see no change with this PR.

Uhm but run callback relies on run fork, so it should be affected as well right?

[TylorS]

This PR enables non-immediate starts through immediate:false as the default is true to avoid breaking changes

[mattiamanzati]

enables

Ohh, I though of it in the completely opposite direction.

[mikearnaldi]

Can we add back Scope in the style used by forkIn? also it is worth updating the main runFork method in Effect.ts to take the RunOptions currently only Runtime exposes it

[TylorS]

@mikearnaldi PR Updated with Scope here and the additional options to Effect.runFork here

[tim-smart]

@TylorS if you rebase that should fix CI

[TylorS]

Rebased