Skip to content
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

journal: close unix socket after successful probe #367

Merged
merged 1 commit into from
Apr 15, 2021

Conversation

jeremy-gill
Copy link
Contributor

This is done to ensure unix sockets aren't left dangling after each Enabled() call, which
can lead to eventual process resource exhaustion. Fixes #366.

@lucab lucab changed the title Journald: Close unix socket after use. journald: close unix socket after successful probe Apr 15, 2021
@lucab
Copy link
Contributor

lucab commented Apr 15, 2021

Good catch, and thanks for the separate bug report too! It is my understanding that the garbage collector would eventually take care of the stale socket, and that most consumers would do the probe only once upfront and then cache the result.
However I totally see how this could hit the limit of open FDs in a tight loop, and closing the socket when done is a good practice anyway.

@lucab lucab enabled auto-merge April 15, 2021 10:16
@lucab lucab changed the title journald: close unix socket after successful probe journal: close unix socket after successful probe Apr 15, 2021
Copy link
Contributor

@lucab lucab left a comment

Choose a reason for hiding this comment

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

A couple of minor comments on spurious new lines. When fixing those, can you please squash everything into a single final commit?

journal/journal_unix.go Outdated Show resolved Hide resolved
journal/journal_unix.go Show resolved Hide resolved
auto-merge was automatically disabled April 15, 2021 13:08

Head branch was pushed to by a user without write access

@jeremy-gill jeremy-gill force-pushed the journald_unix_socket_leak_fix branch from 2f17ced to 6835f56 Compare April 15, 2021 13:08
This is done to ensure unix sockets aren't left dangling after each Enabled() call, which
can lead to eventual process resource exhaustion. Fixes coreos#366.
@jeremy-gill jeremy-gill force-pushed the journald_unix_socket_leak_fix branch from 6835f56 to 68a12a3 Compare April 15, 2021 13:16
Copy link
Contributor Author

@jeremy-gill jeremy-gill left a comment

Choose a reason for hiding this comment

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

The consumer in question that triggered this issue is zerolog. Zerolog's journald backend writer checks journald status for each write (see here). Sockets quickly pile up. Perhaps there's a better way to ensure the connection is still alive in zerolog.

journal/journal_unix.go Show resolved Hide resolved
@lucab lucab enabled auto-merge April 15, 2021 14:13
@lucab lucab merged commit 2aca22e into coreos:master Apr 15, 2021
@jeremy-gill jeremy-gill deleted the journald_unix_socket_leak_fix branch June 3, 2021 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

journald unix sockets are leaked whenever Enabled() is called
2 participants