Skip to content

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

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