Skip to content

feat: dequeue worker request on timeout #1540

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

Merged
merged 4 commits into from
May 9, 2025
Merged

Conversation

Tolsee
Copy link
Contributor

@Tolsee Tolsee commented May 5, 2025

Description

Fixes (#1539 (comment))

@Tolsee Tolsee marked this pull request as ready for review May 7, 2025 01:21
@Tolsee
Copy link
Contributor Author

Tolsee commented May 7, 2025

Not sure, why the builds are failing.

@AlliBalliBaba
Copy link
Contributor

Seems like this command fails since it tries to update go to 1.24.3, but the toolchain is fixed to 1.24.2 https://github.com/dunglas/frankenphp/blob/1d74b2caa825082623fc115bac329147513b25f4/Dockerfile#L96
@dunglas should it just be 1.24.2 here as well?

@dunglas
Copy link
Member

dunglas commented May 7, 2025

It would be nice to automatically use the latest patch version.

@Tolsee Tolsee changed the title feat: add test case for max_wait_time feat: dequeue worker request on timeout May 9, 2025
@Tolsee
Copy link
Contributor Author

Tolsee commented May 9, 2025

@AlliBalliBaba What do you think about the actual solution?

rw.(http.Flusher).Flush()

if f, ok := rw.(http.Flusher); ok {
f.Flush()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f.Flush()
f.Flush()

@@ -225,6 +225,7 @@ func (worker *worker) handleRequest(fc *frankenPHPContext) {
case scaleChan <- fc:
// the request has triggered scaling, continue to wait for a thread
case <-timeoutChan(maxWaitTime):
metrics.DequeuedWorkerRequest(worker.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good 👍, you can probably also add this line in threadregular.go (for non-worker requests)

@dunglas dunglas merged commit 2f7b987 into php:main May 9, 2025
13 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants