-
Notifications
You must be signed in to change notification settings - Fork 398
refactor: improve ExtensionWorkers API #1952
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the worker extension API from a registration-based pattern to a functional options pattern, replacing RegisterWorker/NewWorker with WithExtensionWorkers.
- Replaces the global worker registry approach with direct option passing via
Init() - Introduces a new
Workersinterface to provide type-safe access to extension workers - Updates the Caddy integration to support the new worker extension API via
RegisterWorkers()
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| workerextension.go | Refactors Worker struct to Workers interface and extensionWorkers implementation, simplifying API by removing error handling from SendMessage |
| workerextension_test.go | Updates tests to use new WithExtensionWorkers() API pattern and modernizes test cleanup with t.Cleanup() |
| options.go | Adds WithExtensionWorkers() function and moves related options for better organization |
| worker.go | Adds assignment of internalWorker reference during worker creation |
| frankenphp.go | Removes global extensionWorkers registry logic from Init() |
| caddy/app.go | Adds RegisterWorkers() function with mutex-protected global options to support extensions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I love how you can provided ini settings and such. Should it be documented somewhere? A new section in the "Writing extensions" doc? |
|
Sure, this needs to be documented. |
|
Actually, max time exceeded errors was currently hidden and undetectable. I updated the code to return them to the calling Go code. cc @AlliBalliBaba |
|
I also changed the error returned when the server is overloaded and refuse request from a 504 "Gateway Timeout" to a 503 "Service Unavailable". It's more appropriate because we're not acting as a proxy but as an application server here. |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
a92a05e to
52fde72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create an issue so we don't forget to document it, if it's not shipped in the PR?
Yet another refactoring and simplification of the extension worker API.
I also pluralized names for consistency with the existing
WithWorkersfunction. I'm not entirely sure about that.This allows us to greatly simplify extension writing and should increase performance a bit.
Here is the patch for
frankenphp-queueto use this new API: https://github.com/dunglas/frankenphp-queue/compare/refactor/ExtensionWorkers?expand=1WDYT?