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

fix for entry data field race condition #1229

Merged
merged 1 commit into from Feb 16, 2021
Merged

fix for entry data field race condition #1229

merged 1 commit into from Feb 16, 2021

Conversation

@dgsb
Copy link
Collaborator

@dgsb dgsb commented Feb 16, 2021

No description provided.

@dgsb dgsb merged commit 88d56b6 into master Feb 16, 2021
3 checks passed
@dgsb dgsb deleted the fix-data-entry-race branch Feb 16, 2021
@dgsb
Copy link
Collaborator Author

@dgsb dgsb commented Feb 16, 2021

Fixes #1217, #1046, #1193, #953

@@ -212,68 +224,68 @@ func (entry Entry) HasCaller() (has bool) {

// This function is not declared with a pointer value because otherwise
Copy link

@epelc epelc Feb 16, 2021

This comment should be updated since it's a pointer method now.

Is it safe to use via pointer?

Copy link
Collaborator Author

@dgsb dgsb Feb 16, 2021

Yes it is safe, the comment should indeed be updated.

if _, err = entry.Logger.Out.Write(serialized); err != nil {
fmt.Fprintf(os.Stderr, "Failed to write to log, %v\n", err)
}
func() {
Copy link

@epelc epelc Feb 16, 2021

This extra function wrapper isn't required here since the write does nothing else after writing. ie the lock doesn't require it.

Copy link
Collaborator Author

@dgsb dgsb Feb 16, 2021

Yes it's a leftover from temporary step, I will remove it

@epelc
Copy link

@epelc epelc commented Feb 16, 2021

@dgsb also noticed the Entry.Dup() method is public. I know you just did a minor release but noticed in a comment on other issue you said there were no api changes.

Not sure if this was intentionally left public or not. Might be soon enough to catch if it wasn't intentional.

@dgsb
Copy link
Collaborator Author

@dgsb dgsb commented Feb 16, 2021

Good catch we should have a bump on the minor, thanks for your thorough review 👍

for k, v := range entry.Data {
data[k] = v
}
return &Entry{Logger: entry.Logger, Data: data, Time: entry.Time, Context: entry.Context, err: entry.err}
Copy link

@shallowclouds shallowclouds Mar 24, 2021

Why not copy caller into new entry? There are some scenarios where I want to pass the caller by myself, for example, many libraries accept a logger interface and use it to output internal log(such as SQL exec log for Gorm), so many users wrap logrus to the needed interface, but the caller got by logrus is inaccuracy, and I get the caller and pass it into Entry, and this change breaks my wrapped logger for these libraries.

Can I send a PR for copying caller in Entry.Dup util? What do you think of it? Thanks! @dgsb @epelc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants