refactor: decouple worker threads from non-worker threads #1137
+623
−340
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # frankenphp.c # frankenphp.go # php_thread.go # worker.go
Sec32fun32
approved these changes
Nov 4, 2024
Sec32fun32
approved these changes
Nov 4, 2024
|
Hmm that segfault is interesting, It's probably not fully safe to execute a PHP script while calling |
dunglas
reviewed
Nov 4, 2024
worker.go
Outdated
| // when frankenphp_finish_request() is directly called from PHP | ||
| // | ||
| //export go_frankenphp_finish_request_manually | ||
| func go_frankenphp_finish_request_manually(threadIndex C.uintptr_t) { |
There was a problem hiding this comment.
Suggested change
| func go_frankenphp_finish_request_manually(threadIndex C.uintptr_t) { | |
| func go_frankenphp_php_finish_request(threadIndex C.uintptr_t) { |
?
I'm not fond of the name I suggest, but "manually" sounds a bit weird.
There was a problem hiding this comment.
Yeah I guess you're right 😅 . I changed it to go_frankenphp_finish_php_request
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors how threads are started and is meant as a step towards scaling threads at runtime.
How worker threads are currently started:
Currently, worker threads are started from regular threads via sending a special request
to ServeHTTP. The disadvantage here is that between sending the special worker request
and receiving it, we are losing control over which thread becomes a worker thread.
Worker threads and regular threads are inevitable coupled to each other.
How worker threads are started with this PR:
This PR decouples worker threads from regular threads and makes the
php_threadstructa wrapper around the thread's lifetime.
A 'PHP thread' is currently just a
pthreadwith its ownTSRMstorage (this doesn'tnecessarily have to be tied to a real thread in the future as discussed in #1090).
The thread starts, does some work in a loop and then stops. This PR makes it possible
to configure these 3 lifetime hooks from the go side via the
php_threadstruct:This allows re-using the same mechanism for regular threads as well as worker threads.
It also makes it easier to create other potential types of threads in the future
(like 'scheduled workers' or 'task workers').
Additionally, it now would also be possible to grab an 'idle thread', exchange it's hooks and
turn it into a different type of thread at runtime without stopping the underlying thread.
(This PR doesn't go that far though)