Skip to content
This repository was archived by the owner on Jun 19, 2025. It is now read-only.

Conversation

@aead
Copy link
Member

@aead aead commented Jun 17, 2024

This commit fixes a goroutine leak that occurs when reloading the server configuration.

During a config reload, the server establishes a 2nd connection to the backend keystore and replaces the existing connection with the newly opened one. The switch is performed atomically (without locking) to not block or abort ongoing requests.

Once the server has replaced the keystore connection, it closes it. Before this commit, the server stopped the in-memory key cache and its GC goroutines. However, it did not close any resources (goroutines/file descriptors) allocated by the replaced keystore. This commit fixes this.

The resource leak can only be observed when reloading the KES server config - i.e. using SIGHUP signals

@aead aead requested review from donatello and shtripat June 17, 2024 15:21
This commit fixes a goroutine leak that occurs when
reloading the server configuration.

During a config reload, the server establishes a 2nd
connection to the backend keystore and replaces the
existing connection with the newly opened one. The switch
is performed atomically (without locking) to not block
or abort ongoing requests.

Once the server has replaced the keystore connection,
it closes it. Before this commit, the server stopped
the in-memory key cache and its GC goroutines. However,
it did not close any resources (goroutines/file descriptors)
allocated by the replaced keystore. This commit fixes this.

Signed-off-by: Andreas Auernhammer <[email protected]>
Copy link
Contributor

@klauspost klauspost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@harshavardhana harshavardhana merged commit 12195cc into master Jun 17, 2024
@harshavardhana harshavardhana deleted the keystore-leak branch June 17, 2024 15:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants