fix: reuse created-once channels so queued requests don't get stuck on replaced channels#2470
fix: reuse created-once channels so queued requests don't get stuck on replaced channels#2470henderkes wants to merge 3 commits into
Conversation
henderkes
force-pushed
the
fix/reload_timeout
branch
from
777ea43 to
c7ec04a
Compare
henderkes
force-pushed
the
fix/reload_timeout
branch
from
a41001f to
fdd8f60
Compare
| if regularRequestChan == nil { | ||
| regularRequestChan = make(chan contextHolder) | ||
| } |
There was a problem hiding this comment.
I wonder if there are any edge cases where this would be bad if the Caddy config changes on reload. The queued requests might be from a previous server configuration.
There was a problem hiding this comment.
They'd be handled by the new Caddyfile, while previously they'd starve without being assigned a thread to handle them. The switch from new to old config has to happen at some point, don't think there's a way to handle it perfectly predictable with concurrently incoming requests, detecting when a request would have to be handled by the already-shutdown one would be out of scope.
There was a problem hiding this comment.
Hmm yeah the fact that this might cause requests to bleed across different configurations is pretty unfortunate.
Ideally there would be a way to detect if we're just doing a forced reload without any config change, then we could just use the reboot logic from #2364 instead of this shutdown:
Line 173 in edaffab
There was a problem hiding this comment.
No, I think we'd need to restart the whole php engine for opcache and realpath to get rebuilt. That's the force reload thing again.
I'm also not sure it's a real issue - if someone does a config change + reload they need to know that concurrently incoming requests will either be handled by the old or the new runtime/Caddyfile. If they do backwards incompatible changes... well, they'd fail after the reload anyhow.
There was a problem hiding this comment.
That might be hard to do though assuming service frankenphp reload does the same thing as a forced reload via admin api.
|
|
||
| maxWaitTime time.Duration | ||
| // atomic: read by in-flight requests while a reload may rewrite it | ||
| maxWaitTime atomic.Int64 |
There was a problem hiding this comment.
Does this need to be atomic? I think it's at least guaranteed that there are no incoming requests during a reload. The ones still queued in the timeout channel are probably arriving right before shutdown (at least that's what I'm assuming)
There was a problem hiding this comment.
Technically of course not, reads/writes are atomic on aarch64/x86_64 anyways, but Go's race detector complained. That's the only reason I changed it.
2 participants
fixes #2469