refactor: improve Worker public API and docs

[dunglas]

First step for #1883 (comment)

[dunglas]

What's the use case of this outside of tests @withinboredom? It looks unused

[withinboredom]

It's called once a thread is ready to accept work. It makes more sense in the case of thread autoscaling, since you may need to handle threads going away and/or coming online.

[dunglas]

I mean the atomic int, not the method.

[withinboredom]

oh, yeah. that's only used for tests 🤦

[dunglas]

Shouldn't we define a MaxThreads() method too, at least in the interface, to prevent a future BC break that we can prevent?

[withinboredom]

These threads don't autoscale, so if someone returns 4 min threads and 8 max threads because it is on the interface, it won't do anything. Once we add autoscaling, then it makes more sense to have a MaxThreads(). Whether we want that change to be a breaking one or not, is a good question. :)

It might be a good idea to add it to the interface with one of two options (I'm partial to the second option):

1. simply ignore max threads
2. log a warning if they return different settings, something like: extension "name" has configured different min/max threads but autoscaling is not implemented. min threads will be used.

[AlliBalliBaba]

I'm a bit late to the review, but why expose the interface at all, won't it break on any change in the api?

[dunglas]

I'm not sure of what you mean. How could the extensions work if there is no public interface to implement?

[AlliBalliBaba]

There would be less potential for breakage by just exposing a worker struct

type Worker struct {
  onReady func()
  onShutdown func()
 ....
}

Even better would be sticking to how other library options are exposed.

worker := NewExternalWorker(
"fileName", 
10, 
injectRequest, 
WithWorkerOnReady(..), 
WithWorkerX(...),
)

[withinboredom]

It's called once a thread is ready to accept work. It makes more sense in the case of thread autoscaling, since you may need to handle threads going away and/or coming online.

[withinboredom]

These threads don't autoscale, so if someone returns 4 min threads and 8 max threads because it is on the interface, it won't do anything. Once we add autoscaling, then it makes more sense to have a MaxThreads(). Whether we want that change to be a breaking one or not, is a good question. :)

It might be a good idea to add it to the interface with one of two options (I'm partial to the second option):

1. simply ignore max threads
2. log a warning if they return different settings, something like: extension "name" has configured different min/max threads but autoscaling is not implemented. min threads will be used.

[AlliBalliBaba]

This method is not in use anywhere internally, why expose it in the interface?

[dunglas]

To allow things like this: https://github.com/dunglas/frankenphp-grpc/blob/main/grpc.go#L10

[AlliBalliBaba]

Just confused about GetRequest and SendRequest. Wouldn't you always just want to send a message?

[AlliBalliBaba]

IMO it would be much more elegant if there only was a SendRequest that just blocks until the message is finished. The user can call go SendRequest() if they want it to be non-blocking.

Needs less goroutines and allows writing more straightforward code. Instead of using AfterFunc for handling the return value, SendRequest can just return it directly or an error if something fails. AfterFunc feels like doing async-await in go 😅

[AlliBalliBaba]

Aren't we essentially just registering a worker here? Wouldn't it make sense to just pass worker options like in WithWorkers?

WithWorkers(name string, fileName string, num int, options ...WorkerOption)

[dunglas]

The worker can contain custom Go code like: https://github.com/dunglas/frankenphp-queue/blob/main/worker.go

We could maybe indeed pass a WithWokers option instead of having this global method, however. WDYT @withinboredom?