Hi,
Varnish has a limit of 8192 characters per line (so per-command in our case). You are supposed to be able to raise this with param.set cli_buffer [x], but this doesn't seem to work. https://www.varnish-cache.org/trac/ticket/772 seems to state that longer commands are only allowed in authenticated mode with the new heredoc syntax, raising the question of what the cli_buffer param actually does...
Anyway, I've had trouble with the expire module purging an entire menu with hundreds of nodes in it, and Varnish hanging up the socket after more than 8192 bytes of regex were pushed at it in a single command.
I've created a patch that causes varnish_expire_cache() to batch the varnish_purge() commands into regexes not too much longer than 7500 bytes each to prevent this problem. You do end up with multiple bans in your ban list if it's there's a lot of paths, but at least it's a compatible way to prevent hitting the command length limit.
Unfortunately this only protects in the case of Expire module usage. Direct calls into varnish_purge() may still fail if they're too long, as the regex is compiled elsewhere.
Hope this helps someone!
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | varnish_purge_limit-1481136-23.patch | 765 bytes | das-peter |
| #20 | varnish_purge_limit-1481136-11.patch | 758 bytes | brettdonohoo |
| #5 | varnish-maxcmdlength-1481136-5.patch | 2.3 KB | neilnz |
| #4 | varnish.diff | 1.62 KB | Eugene Fidelin |
| varnish-maxcmdlength.patch | 1.13 KB | neilnz |
Comments
Comment #1
fabsor commentedWe have support for issuing multiple commands within one connection, maybe we could use that somehow? I guess we like this to work everywhere, not just only for expire.
Comment #2
neilnz commentedSure, that would be nice, but would require refactoring the building of regexes to somewhere further down the chain. Maybe make varnish_purge() optionally take an array in place of $pattern, and build the regexp itself, taking care to limit it appropriately.
The other option would be to parse out the regexp in varnish_purge() and split it there, but that's really ugly.
I've noticed that my patch doesn't always solve the problem where there are a lot of short paths, as the 3 characters added by the regexp in between each path are not counted. When you have hundreds of paths, the bytes taken up by these regexp characters become significant and push the string over the limit again. I've got a revised patch to fix that up, but maybe we should have a look at implementing it down in varnish_purge() instead?
Happy to have a go at a patch along those lines if you like?
Comment #3
fabsor commentedI think your solution sounds like a good one. Let's try to get that logic into varnish_purge. I'm putting this into needs work for now, thanks for working on this!
Comment #4
Eugene Fidelin commentedI propose another solution.
In varnish_expire_cache () we need to divide $paths array into chunks by 30 items
In this case we will exceed the maximum length of command (8000 characters) only if all urдs has length more than 250 characters - this is unlikely.
Of course user can specify other chunk size in settings
Comment #5
neilnz commentedHere is an improved patch that adds a varnish_purge_paths() function that takes care of building the regex. varnish_expire_cache() now calls this, rather than varnish_purge() directly. The varnish_purge() function still works the same way, so won't break any other modules calling into this directly.
Comment #6
neilnz commented... and I just realised that the Drush command calls the varnish_expire_cache() hook directly. I've changed it to call varnish_purge_paths() instead.
Here's an updated patch for this.
Comment #7
pjcdawkins commentedReroll for D7.
Comment #8
millenniumtreeGlad to see this addressed - just had a problem with our very large multi-domain site. One particular page was expiring over 11,000 individual URLs.
Must-have for the D6 release!
Comment #9
pjcdawkins commentedRerolled for latest -dev / -beta2.
Comment #10
patrick_durold#9 will not apply to varnish 7.x-1.0-beta2 and contains mostly idempotent hunks anyway. Patch redone and tested with beta2.
Comment #11
vincenzo commentedPatch #10 works beautifully for me. I am keen to see this merged in.
Comment #12
pjcdawkins commentedYes patch #10 works well.
Comment #13
imclean commentedPatch in #6 resolved the issue for me with 6.x-1.x-dev.
Comment #14
vincenzo commentedAny chance we are going to see this merged in? Please.
(#10 works flawlessly)
Comment #15
JeremyFrench commentedhttp://drupalcode.org/project/varnish.git/commit/d4cddfe
Pushed to 7.x-1.x branch.
Comment #16
JeremyFrench commentedComment #18
brettdonohoo commentedI think this problem re-emerged after this patch.
http://cgit.drupalcode.org/varnish/commit/?id=6412c501e71cbaf39316ca3ee3...
I'm getting this same error in 7.x-1.0-beta3.
Comment #19
brettdonohoo commentedComment #20
brettdonohoo commentedReroll for beta3.
Comment #21
brettdonohoo commentedComment #22
jwintx commentedPatch #20 resolved the issue for me in beta3
Comment #23
das-peter commentedRe-rolled for 7.x-1.x-dev, seems to work as intended and mainly re-introduces a lost approach.
So I'd say RTBC.
Comment #24
misc commentedThanks. Pushed to latest dev, will be in beta5.
Comment #26
misc commentedCorrection - will be in 7-x.1.1