panic if `Run` and `Stop` race

[twmb]

internal_worker.AggregatedWorker#Run calls aw.Start, which asserts that the worker has not yet been stopped.

Run is a blocking function, it must be executed in a goroutine if the user wants to stop the worker independently. The user may then wait for an input signal and Stop the worker. It is possible that Run has not yet started and Stop executes before Run. In this tight sequence of events, the code will panic:

sdk-go/internal/internal_worker.go

Line 955 in dea0cfa

panic("attempted to start a worker that has been stopped before")

Since Run returns an error, I'd expect this code to just return ErrClosed or something similar.

[cretz]

For users who cannot be sure that worker.Stop() will be called after worker.Run() has had enough chance to get started, they may find it better to instead use worker.Start() in the same goroutine they may later call Stop(). This is one reason we provide both Start() and Run(), for those that need to now when started (which Run() cannot tell them).

Having said that, yes, we can return an error for Start() and Run() if they are called on a stopped worker. Still, most people should tailor their code to avoid this possibility.

[twmb]

Is the point of Run to not ever be shutdown? The theoretical race exists in any model of using Run in tandem with Close unless some activity that is being signals readiness such that the worker can be stopped -- but this relies on input.

[cretz]

Definitely can shutdown. The point of Run is as a helper for people that want to block until shutdown, but Start is better for those concerned that they need to know when start completed because they may immediately shutdown.

Yes, the race exists between Run and Stop. Users concerned about this race should use Start and Stop.

[cretz]

Closing, in favor of using Start. Users should not use Run if they're concerned they may shutdown before running. Reopen if needed.

[twmb]

At a minimum, the panic should be noted in docs on Run. The safest way to program around the panic right now is to only allow Close to occur in an activity that is ran at least once. This is the only way for a user to _know_ that the temporal client has started. Without this, users have to rely on heuristics and hope. Any proper shutdown path that triggers a more immediate exit can panic if an interrupt is received while things are starting up.

…

On Mon, Aug 8, 2022 at 07:31 Chad Retz ***@***.***> wrote: Closed #868 <#868> as completed. — Reply to this email directly, view it on GitHub <#868 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJZIJNIYQSJTCXFCTN4Q3LVYED35ANCNFSM54XNBYJA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[cretz]

At a minimum, the panic should be noted in docs on Run.

Ok, I will add docs.

The safest way to program around the panic right now is to only allow Close to occur in an activity that is ran at least once

The safest way is to use Start. Users can rely on this Start instead of heuristics and hope. This is a way for users to know Temporal client has started. In retrospect, maybe we should have never had the Run helper since its utility seems confusing and is easy to build yourself using Start.

Just use Start.