-
Notifications
You must be signed in to change notification settings - Fork 463
suggestion: external worker api #1928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
2f3d3c9
4749c8f
f720a9e
2c3f3f2
c80a0b4
a467962
7247c1c
7e40fa8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| <?php | ||
|
|
||
| while (frankenphp_handle_request(function ($message) { | ||
| echo $message; | ||
| return "received message: $message"; | ||
| })) { | ||
| // continue handling requests | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,8 @@ | ||
| package frankenphp | ||
|
|
||
| import ( | ||
| "context" | ||
| "log/slog" | ||
| "errors" | ||
| "net/http" | ||
| "sync" | ||
| "sync/atomic" | ||
| ) | ||
|
|
||
| // EXPERIMENTAL: Worker allows you to register a worker where, instead of calling FrankenPHP handlers on | ||
|
|
@@ -27,154 +24,73 @@ import ( | |
| // Extension workers receive the lowest priority when determining thread allocations. If MinThreads cannot be | ||
| // allocated, then FrankenPHP will panic and provide this information to the user (who will need to allocate more | ||
| // total threads). Don't be greedy. | ||
| type Worker interface { | ||
| // Name returns the worker name | ||
| Name() string | ||
| // FileName returns the PHP script filename | ||
| FileName() string | ||
| // Env returns the environment variables available in the worker script. | ||
| Env() PreparedEnv | ||
| // MinThreads returns the minimum number of threads to reserve from the FrankenPHP thread pool. | ||
| // This number must be positive. | ||
| MinThreads() int | ||
| // OnReady is called when the worker is assigned to a thread and receives an opaque thread ID as parameter. | ||
| // This is a time for setting up any per-thread resources. | ||
| OnReady(threadId int) | ||
| // OnShutdown is called when the worker is shutting down and receives an opaque thread ID as parameter. | ||
| // This is a time for cleaning up any per-thread resources. | ||
| OnShutdown(threadId int) | ||
| // OnServerShutdown is called when FrankenPHP is shutting down. | ||
| OnServerShutdown(threadId int) | ||
| // GetRequest is called once at least one thread is ready. | ||
| // 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) | ||
| } | ||
|
|
||
| // EXPERIMENTAL: WorkerRequest represents a request to pass to a worker script. | ||
| type WorkerRequest struct { | ||
| // Request is an optional HTTP request for your worker script to handle | ||
| Request *http.Request | ||
| // Response is an optional response writer that provides the output of the provided request, it must not be nil to access the request body | ||
| Response http.ResponseWriter | ||
| // CallbackParameters is an optional field that will be converted in PHP types or left as-is if it's an unsafe.Pointer and passed as parameter to the PHP callback | ||
| CallbackParameters any | ||
| // AfterFunc is an optional function that will be called after the request is processed with the original value, the return of the PHP callback, converted in Go types, is passed as parameter | ||
| AfterFunc func(callbackReturn any) | ||
| type Worker struct { | ||
| name string | ||
| fileName string | ||
| num int | ||
| options []WorkerOption | ||
| } | ||
|
|
||
| var extensionWorkers = make(map[string]Worker) | ||
| var extensionWorkersMutex sync.Mutex | ||
|
|
||
| // EXPERIMENTAL: RegisterWorker registers a custom worker script. | ||
| func RegisterWorker(worker Worker) { | ||
| extensionWorkersMutex.Lock() | ||
| defer extensionWorkersMutex.Unlock() | ||
|
|
||
| extensionWorkers[worker.Name()] = worker | ||
| extensionWorkers[worker.name] = worker | ||
|
AlliBalliBaba marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // startWorker creates a pipe from a worker to the main worker. | ||
| func startWorker(w *worker, extensionWorker Worker, thread *phpThread) { | ||
| for { | ||
| rq := extensionWorker.GetRequest() | ||
|
|
||
| var fc *frankenPHPContext | ||
| if rq.Request == nil { | ||
| fc = newFrankenPHPContext() | ||
| fc.logger = logger | ||
| } else { | ||
| fr, err := NewRequestWithContext(rq.Request, WithOriginalRequest(rq.Request)) | ||
| if err != nil { | ||
| logger.LogAttrs(context.Background(), slog.LevelError, "error creating request for external worker", slog.String("worker", w.name), slog.Int("thread", thread.threadIndex), slog.Any("error", err)) | ||
| continue | ||
| } | ||
|
|
||
| var ok bool | ||
| if fc, ok = fromContext(fr.Context()); !ok { | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| fc.worker = w | ||
|
|
||
| fc.responseWriter = rq.Response | ||
| fc.handlerParameters = rq.CallbackParameters | ||
|
|
||
| // Queue the request and wait for completion if Done channel was provided | ||
| logger.LogAttrs(context.Background(), slog.LevelInfo, "queue the external worker request", slog.String("worker", w.name), slog.Int("thread", thread.threadIndex)) | ||
|
|
||
| w.requestChan <- fc | ||
| if rq.AfterFunc != nil { | ||
| go func() { | ||
| <-fc.done | ||
|
|
||
| if rq.AfterFunc != nil { | ||
| rq.AfterFunc(fc.handlerReturn) | ||
| } | ||
| }() | ||
| } | ||
| } | ||
| } | ||
| // EXPERIMENTAL: SendRequest sends an HTTP request to the worker and writes the response to the provided ResponseWriter. | ||
| func (w Worker) SendRequest(rw http.ResponseWriter, r *http.Request) error { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we mean to make the receiver here a non-pointer? I'm not 100% sure, but this would mean (potentially) a copy needs to be made every time a message is sent.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it won't need to make a copy, the compiler should be smart enough in that case. The struct would only be copied if it's modified.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm 99% sure Go doesn't have COW semantics. In any case, the struct header will def be copied each time these are called, and put on the stack. Letting it live in the heap would likely be kept in an L1/L2 cache. If we can kick out the options slice (make it a part of registration), it can fit safely into a cache line and basically be "free" (and not even matter which type of receiver we use).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyway, that's a micro-optimisation (though this is on a hot-path). I think I have a good idea here, I'll create a PR to this PR -- but I do need to leave for a date in the next hour. I think this is fine for now, so it isn't a blocker from merging.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hard to guess what the CPU will actually do here without benchmarks, but I'm fine with making this a pointer either way, I'll wait for your PR👍 |
||
| worker := getWorkerByName(w.name) | ||
|
|
||
| // EXPERIMENTAL: NewWorker creates a Worker instance to embed in a custom struct implementing the Worker interface. | ||
| // The returned instance may be sufficient on its own for simple use cases. | ||
| func NewWorker(name, fileName string, minThreads int, env PreparedEnv) Worker { | ||
| return &defaultWorker{ | ||
| name: name, | ||
| fileName: fileName, | ||
| env: env, | ||
| minThreads: minThreads, | ||
| requestChan: make(chan *WorkerRequest), | ||
| activatedCount: atomic.Int32{}, | ||
| drainCount: atomic.Int32{}, | ||
| if worker == nil { | ||
| return errors.New("worker not found: " + w.name) | ||
| } | ||
| } | ||
|
|
||
| type defaultWorker struct { | ||
| name string | ||
| fileName string | ||
| env PreparedEnv | ||
| minThreads int | ||
| requestChan chan *WorkerRequest | ||
| activatedCount atomic.Int32 | ||
| drainCount atomic.Int32 | ||
| } | ||
| fr, err := NewRequestWithContext( | ||
| r, | ||
| WithOriginalRequest(r), | ||
| WithWorkerName(w.name), | ||
| ) | ||
|
|
||
| func (w *defaultWorker) Name() string { | ||
| return w.name | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| func (w *defaultWorker) FileName() string { | ||
| return w.fileName | ||
| } | ||
| err = ServeHTTP(rw, fr) | ||
|
|
||
| func (w *defaultWorker) Env() PreparedEnv { | ||
| return w.env | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| func (w *defaultWorker) MinThreads() int { | ||
| return w.minThreads | ||
| return nil | ||
| } | ||
|
|
||
| func (w *defaultWorker) OnReady(_ int) { | ||
| w.activatedCount.Add(1) | ||
| } | ||
| // EXPERIMENTAL: SendMessage sends a message to the worker and waits for a response. | ||
| func (w Worker) SendMessage(message any, rw http.ResponseWriter) (any, error) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
| internalWorker := getWorkerByName(w.name) | ||
|
|
||
| func (w *defaultWorker) OnShutdown(_ int) { | ||
| w.drainCount.Add(1) | ||
| } | ||
| if internalWorker == nil { | ||
| return nil, errors.New("worker not found: " + w.name) | ||
| } | ||
|
|
||
| func (w *defaultWorker) OnServerShutdown(_ int) { | ||
| w.drainCount.Add(-1) | ||
| w.activatedCount.Add(-1) | ||
| } | ||
| fc := newFrankenPHPContext() | ||
| fc.logger = logger | ||
| fc.worker = internalWorker | ||
| fc.responseWriter = rw | ||
| fc.handlerParameters = message | ||
|
|
||
| internalWorker.handleRequest(fc) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously, there was a way to signal to the external worker that we were ready to receive requests, but now it only applies if the user knew to pass an option in. Can we safely handle the case where we start receiving messages/requests before the worker is ready? If I understand correctly, this would just end up forwarding to a non-worker thread?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah you mean that I think we might need a |
||
|
|
||
| func (w *defaultWorker) GetRequest() *WorkerRequest { | ||
| return <-w.requestChan | ||
| return fc.handlerReturn, nil | ||
| } | ||
|
|
||
| func (w *defaultWorker) SendRequest(r *WorkerRequest) { | ||
| w.requestChan <- r | ||
| // EXPERIMENTAL: NewWorker creates a Worker instance to embed in a custom struct implementing the Worker interface. | ||
| // The returned instance may be sufficient on its own for simple use cases. | ||
| func NewWorker(name string, fileName string, num int, options ...WorkerOption) Worker { | ||
| return Worker{ | ||
| name: name, | ||
| fileName: fileName, | ||
| num: num, | ||
| options: options, | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to add back some of the original methods from the original interface:
Now it is basically an opaque token for the extension (they can't access these fields) and they have to keep track of the values they passed themselves which just increases memory usage.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure, I'd rather keep the api minimal. Methods like these can be added anytime afterwards if necessary without breaking the api.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here’s an actual use case: I’m working on a NATs Jetstream client, which has headers/bodies/etc -- so I need requests, not messages. There are also different types of workers (key/value change workers and stream workers). These details are needed after initialisation. I can’t assume that the number of threads is the actual number of threads I requested. I need access to that value to ensure I initialise the appropriate number of consumers. I can’t assume that the worker name is the same worker name I requested either, and I’d like my metrics to match frankenphp’s without hardcoding things. Being that I know how FrankenPHP works, I can kinda cheat, but I’m trying to also work out "best practices" instead of cheating.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since we are returning by value, we can just export the fields, maybe?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, we actually won't be able to see the worker configuration until after registration, but since it is a value, it won't have the most updated information. maybe this is a non-issue for now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm in that case That also makes me think: wouldn't the extension workers currently drain threads from the general threadpool? So if someone uses an extension, all their threads might be unknowingly consumed by the extension. Seems kinda messy, the extension threads should probably be counted on-top of the configured
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Heh, yeah, that's why the comment said: "don't be greedy". I would expect extension developers using this would have a way to define how many workers to consume, and it is up to the operator to do a bit of math to ensure there are either enough threads configured or enough CPUs allocated.
I don't think it is a good idea to try and be smart about this. Setting the number of threads in the caddyfile isn't hard.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm we can leave it as-is for now, maybe extensions will make that configurable anyways. It might also make sense in the future to have a per-worker |
||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.