refactor: improve Worker public API and docs by dunglas · Pull Request #1915

refactor: improve Worker public API and docs by dunglas · Pull Request #1915 · php/frankenphp


		func (w *defaultWorker) ThreadActivatedNotification(_ int) {
		func (w *defaultWorker) OnReady(_ int) {
		w.activatedCount.Add(1)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the atomic int, not the method.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

		InjectRequest(r *WorkerRequest)
		// MinThreads returns the minimum number of threads to reserve from the FrankenPHP thread pool.
		// This number must be positive.
		MinThreads() int

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

simply ignore max threads
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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...),
)


		func (w *defaultWorker) ThreadActivatedNotification(_ int) {
		func (w *defaultWorker) OnReady(_ int) {
		w.activatedCount.Add(1)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

		InjectRequest(r *WorkerRequest)
		// MinThreads returns the minimum number of threads to reserve from the FrankenPHP thread pool.
		// This number must be positive.
		MinThreads() int

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

simply ignore max threads
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.

		// The returned request will be passed to the worker script.
		GetRequest() *WorkerRequest
		// SendRequest sends a request to the worker script. The callback function of frankenphp_handle_request() will be called.
		SendRequest(r *WorkerRequest)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅


		// EXPERIMENTAL
		// EXPERIMENTAL: RegisterWorker registers a custom worker script.
		func RegisterWorker(worker Worker) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dunglas deleted the refactor/external-worker-api branch

October 29, 2025 10:36