-
Notifications
You must be signed in to change notification settings - Fork 188
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
feat(caddy): persist replay cache across config reloads #212
Conversation
…r proxy protocol.
43318e7
to
9de7a6a
Compare
9de7a6a
to
d435603
Compare
Picking this up again. PTAL @fortuna |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
service/replay.go
Outdated
// archive. | ||
func (c *ReplayCache) Resize(capacity int) { | ||
if capacity > MaxCapacity { | ||
panic("ReplayCache capacity would result in too many false positives") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please return an error and handle it instead of crashing the server. Config updates shouldn't crash the server (perhaps log error and keep the old config).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this to mimic the constructor's behavior. Do you want me to update it there as well or leave that one alone, since that's more likely to be called on startup by a caller?
fe8d6ca
to
8209056
Compare
@@ -92,11 +93,25 @@ func (c *ReplayCache) Add(id string, salt []byte) bool { | |||
return false | |||
} | |||
_, inArchive := c.archive[hash] | |||
if len(c.active) == c.capacity { | |||
if len(c.active) >= c.capacity { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fortuna I already merged a bit too fast, but just wanted to point out this specific change I made as part of not actively shrinking the maps in the Resize()
method. This is needed, otherwise we'll never shrink maps that were resized down.
As the replay history config could be changed, this introduces a way to resize the
ReplayCache
in case the capacity needs to shrink.