fix(MemcachedFactory): Enable TCP_NODELAY and adjust default timeouts to be reasonable - #62670
Conversation
… to be reasonable Signed-off-by: David Dreschner <david.dreschner@nextcloud.com>
DerDreschner
added
3. to review
|
/backport to stable34 |
|
/backport to stable33 |
|
/backport to stable32 |
|
/backport to stable31 |
|
/backport to stable30 |
|
/backport to stable29 |
|
Backport down to stable29 happens to stabilize CI runs on GitHub. Previous branches don't have the memcached PHPUnit tests using GitHub Actions. |
There was a problem hiding this comment.
Thanks for picking up that ancient ticket, seems reasonable to adjust the limits 👍
nit: Using the class name as scope is not ideal. The scope is meant to describe the area/module/subsystem affected. Hence I would suggest cache or memcache. Which files are changed are visible from the diff already, no need to repeat that in the commit message.
|
The backport to # Switch to the target branch and update it
git checkout stable29
git pull origin stable29
# Create the new backport branch
git checkout -b backport/62670/stable29
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 308240aa
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/62670/stable29Error: Failed to check for changes with origin/stable29: No changes found in backport branch Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports. |
|
The backport to # Switch to the target branch and update it
git checkout stable30
git pull origin stable30
# Create the new backport branch
git checkout -b backport/62670/stable30
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 308240aa
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/62670/stable30Error: Failed to check for changes with origin/stable30: No changes found in backport branch Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports. |
|
The backport to # Switch to the target branch and update it
git checkout stable31
git pull origin stable31
# Create the new backport branch
git checkout -b backport/62670/stable31
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 308240aa
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/62670/stable31Error: Failed to check for changes with origin/stable31: No changes found in backport branch Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports. |
|
The backport to # Switch to the target branch and update it
git checkout stable32
git pull origin stable32
# Create the new backport branch
git checkout -b backport/62670/stable32
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 308240aa
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/62670/stable32Error: Failed to check for changes with origin/stable32: No changes found in backport branch Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports. |
|
The backport to # Switch to the target branch and update it
git checkout stable33
git pull origin stable33
# Create the new backport branch
git checkout -b backport/62670/stable33
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 308240aa
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/62670/stable33Error: Failed to check for changes with origin/stable33: No changes found in backport branch Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports. |
|
The backport to # Switch to the target branch and update it
git checkout stable34
git pull origin stable34
# Create the new backport branch
git checkout -b backport/62670/stable34
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 308240aa
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/62670/stable34Error: Failed to check for changes with origin/stable34: No changes found in backport branch Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports. |
True, would have changed that if I were quicker then the auto-merge. Will do better in the future! As it's clear that it affects memcached, I think it's kinda okay-ish for the backports to keep it for consistency. |
There was a problem hiding this comment.
🟡 Not ready to approve
The default OPT_RETRY_TIMEOUT option was dropped (and its units/semantics differ), so the resulting retry/backoff behavior change needs explicit clarification/alignment before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates Nextcloud’s default \Memcached client configuration to reduce sporadic CI/test failures under load by increasing timeouts and enabling TCP_NODELAY for lower latency with small request/response payloads.
Changes:
- Increase default connect and poll timeouts from 50ms to 500ms.
- Increase default send/receive timeouts to 500ms (expressed as
500000microseconds for the memcached extension). - Enable
\Memcached::OPT_TCP_NODELAYby default.
File summaries
| File | Description |
|---|---|
| lib/private/Memcache/MemcachedFactory.php | Adjusts Memcached default socket/poll timeouts and enables TCP_NODELAY in the singleton factory options. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| $defaultOptions = [ | ||
| \Memcached::OPT_CONNECT_TIMEOUT => 50, | ||
| \Memcached::OPT_RETRY_TIMEOUT => 50, | ||
| \Memcached::OPT_SEND_TIMEOUT => 50, | ||
| \Memcached::OPT_RECV_TIMEOUT => 50, | ||
| \Memcached::OPT_POLL_TIMEOUT => 50, | ||
| // Set connect and polling timeouts to 500 milliseconds | ||
| \Memcached::OPT_CONNECT_TIMEOUT => 500, | ||
| \Memcached::OPT_POLL_TIMEOUT => 500, | ||
|
|
||
| // Set send and receive timeouts to 500000 microseconds | ||
| \Memcached::OPT_SEND_TIMEOUT => 500000, | ||
| \Memcached::OPT_RECV_TIMEOUT => 500000, |
There was a problem hiding this comment.
This is intentional as the OPT_RETRY_TIMEOUT is 2 seconds per default and I don't see a reason to set another default value here when no one else is doing it.
There was a problem hiding this comment.
Then config.sample.php should be updated accordingly.
| // Disable Nagle algorithm to speed up connection | ||
| \Memcached::OPT_TCP_NODELAY => true, |
There was a problem hiding this comment.
Yeah, I've meant "reduce latency" with "speed up connection".
Summary
The CI tests against
memcachedfail sporadically. After some research, it's because the default timeouts are way to low for high load scenarios. The ones being used at the moment have no reasoning behind them at all (we use these ones according to the commit message here). This is being mentioned in #14995 and can be observed when comparing our values with those of other PHP applications as well:POLL_TIMEOUTand 4 seconds forCONNECT_TIMEOUTapplyFor our CI tests, the
POLL_TIMEOUTis the one that makes the tests fail. As we're not enablingTCP_NODELAYat the moment, the bare minimum ACK time for small packages (like aHIT MISSwith binary protocol) is 40ms with the default Linux kernel settings. Dangerously close to the 50ms timeout we currently use. Under high load scenarios, the kernel can increase the ACK time to up to 200ms and the RFC even allows up to 500ms.That's why this PR enabled
TCP_NODELAYto speed upHIT MISSscenarios (same as Symfony does, which seem reasonable as well) and adjusts the default timeouts to those being used by MediaWiki as they seem pretty reasonable.Checklist
3. to review, feature component)stable32)AI (if applicable)