⚠️ Fix some more races in the delegating logger #1361
Conversation
|
@alvaroaleman: GitHub didn't allow me to request PR reviews from the following users: charith-elastic. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
cncf-cla: yes
alvaroaleman
force-pushed
the
more-racing
branch
4 times, most recently
from
6970aa9
to
8992715
Compare
k8s-ci-robot
added
size/L
|
Seems to work. Thanks @alvaroaleman If I understand your change correctly, wouldn't it require a lock to be obtained on every logging call even after the promise is fulfilled? Wouldn't that add some overhead? |
alvaroaleman
force-pushed
the
more-racing
branch
from
8992715
to
4209b9d
Compare
Good point. I've replaced the Mutex with a RWMutex, that should minimize the overhead. We will always need something to check if the promise was fulfilled, unfortunately. |
|
WDYT about including this in 0.8? This change is incompatible in that if anyone ever did a |
|
Given that this is a bug that could cause a race condition (and corruption), I'm +1 merging as |
The delegatingLogger had a logger implementation embedded, calling any method of that would race with FullFill(), which replaces the embedded logger.
alvaroaleman
force-pushed
the
more-racing
branch
from
4209b9d
to
ab23736
Compare
alvaroaleman
changed the title
|
Updated the title to be |
|
@alvaroaleman: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| } | ||
|
|
||
| // WithName provides a new Logger with the name appended |
There was a problem hiding this comment.
I guess it doesn't matter because its an interface method
There was a problem hiding this comment.
Err yeah sorry, I abused this to check if the linter correctly doesn't care about godocs of exported methods on unexported types (which it does). Since it is only visible from within the package it doesn't matter much IMO
| l.lock.RLock() | ||
| defer l.lock.RUnlock() |
There was a problem hiding this comment.
Shouldn't these be write locks? Isn't Info writing to the underlying logger here?
There was a problem hiding this comment.
The Lock protects the reference to the logger, not the method call. The actual logger implementation must also be threadsafe, but that needs to be done in that implementation, not here. Everything except Fullfill only reads the reference (as proven by the included tests not failing with a data race)
There was a problem hiding this comment.
If we care about that, I can update the PR later to only use the lock to get the logger reference and not around the whole method call, which might improve performance slightly
There was a problem hiding this comment.
Then I'll keep it as-is, only protecting the reference would require to allocate a pointer so it might actually be worse performance-wise and is more complex
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, estroz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The delegatingLogger had a logger implementation embedded, calling any
method of that would race with FullFill(), which replaces the embedded
logger.
/cc @charith-elastic
Can you verify this fixes all remaining races?
Fixes #1359