Add eager workflow start

[Quinn-With-Two-Ns]

Add eager workflow start

[cretz]

What is the situation you are trying to protect from here?

[Quinn-With-Two-Ns]

a caller accidentally reusing eagerWorkflowExecutor

[cretz]

I think we should be allowed to trust callers on non-exported things, i.e. ourselves, here

[Quinn-With-Two-Ns]

I don't

[cretz]

I am not wondering if there is value in a separate dispatcher and executor concept since this is so straightforward and done on only this call. I wonder if you can just store a eagerWorkflowWorkers sync.Map on WorkflowClient and change this to:

var eagerWorkerInUse eagerWorker
if in.Options.EnableEagerStart && w.client.capabilities.GetEagerWorkflowStart() {
	eagerWorkerIface, _ := w.client.eagerWorkflowWorkers.Load(request.GetTaskQueue().GetName())
	eagerWorkerInUse, _ = eagerWorkerIface.(eagerWorker)
	request.RequestEagerExecution = eagerWorkerInUse != nil && eagerWorkerInUse.tryReserveSlot()
}

And after response:

if request.RequestEagerExecution {
	if task := response.GetEagerWorkflowTask(); task != nil {
		eagerWorkerInUse.processTaskAsync(&eagerWorkflowTask{task}, eagerWorkerInUse.releaseSlot)
	} else {
		eagerWorkerInUse.releaseSlot()
	}
}

[Quinn-With-Two-Ns]

I prefer the abstraction of a separate dispatcher and executor

[cretz]

You took a few lines and made a big file and multiple new types such :-( But it's all subjective, so technically there's no right way I suppose, though I do find inlining simpler in this case.

[cretz]

A sync.Map may be a better choice here, and even if not, you should use a RWMutex and only RLock here, and you should only lock map access, not the slot reservation IMO. (though as mentioned in another comment I think a simple eagerWorkflowWorkers sync.Map on the client itself is probably plenty and the dispatcher/executor is unnecessary)

[Quinn-With-Two-Ns]

Don't believe it is safe to only lock the map access, not the slot reservation. Since the map could be modified while iterating. Originally I thought I might need some limits like eager activity dispatch has, so a mutex made more sense, but I don't think they are needed so I'm fine to use a sync.Map

[cretz]

Slot reservation should be per worker and concurrency safe from a caller POV. Only choosing of a worker I think needs to be synchronized.

[Quinn-With-Two-Ns]

I am just saying I don't you can safety move the mutex in a way that would result in less code being under the critical section, regardless doesn't matter because we are going to using a sync.map

[Quinn-With-Two-Ns]

actually on further review I think a sync.map here is a bad choice because we loose any type safety and insertion would not be safe . So i'll switch to a RWMutex mutex

[cretz]

FWIW, it's ok to lose type safety on sync.Map in this case. It's for internal use. Nobody would ever use sync.Map if they were subject to such requirements even on non-exported fields.

[cretz]

Works for me. Left a couple of optional comments.

[cretz]

A bit scared of accessing this field in a non-thread-safe way, but probably ok since only mutated on start/stop

[rustatian]

@cretz @Quinn-With-Two-Ns Yes, I've found a race here on the roadrunner-temporal tests, on the one where we start and then stop temporal workers from the different goroutines, but protected by mutex from our side. Here is the stack trace of the race:

[Quinn-With-Two-Ns]

@rustatian can you link the roadrunner-temporal test failure? I'll look into it

[rustatian]

@rustatian can you link the roadrunner-temporal test failure? I'll look into it

https://github.com/roadrunner-server/rr-e2e-tests/actions/runs/5790984816/job/15695006300#step:10:963