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

return new entry for Entry.WithContext #941

Merged

Conversation

@hrxu01
Copy link

@hrxu01 hrxu01 commented Mar 29, 2019

Just like Entry.WithError, Entry.WithField and Entry.WithFields, return a new entry for Entry.WithContext. If we don't return a new entry, the old entry with context will be released to entry pool, this may cause some unexpected errors.

@@ -103,8 +103,7 @@ func (entry *Entry) WithError(err error) *Entry {

// Add a context to the Entry.
func (entry *Entry) WithContext(ctx context.Context) *Entry {

Why is the receiver a pointer since you dont manipulate it?

Copy link
Author

@hrxu01 hrxu01 Mar 29, 2019

Keep consistent with Entry.WithError, Entry.WithField and Entry.WithFields methods in the same file.

ok

Copy link
Contributor

@tandr tandr Apr 1, 2019

That pointer means memory barriers will be different when creating a new Entry. I would argue that the rest of these With* calls should have receiver passed by value.

Copy link
Author

@hrxu01 hrxu01 Apr 2, 2019

That pointer means memory barriers will be different when creating a new Entry. I would argue that the rest of these With* calls should have receiver passed by value.

I think having receiver passed by value or pointer are both ok in this case, just make sure to keep consistent. For the present logic, we create a new Entry inside With* methods and copy some but not all fields from receiver, pointer receiver might be slightly better (less and cleaner code).

Copy link
Collaborator

@tgwizard tgwizard left a comment

LGTM, good catch!

@tgwizard tgwizard merged commit a6c0064 into sirupsen:master Mar 31, 2019
2 checks passed
@hrxu01 hrxu01 deleted the return_new_entry_for_Entry.WithContext branch Apr 1, 2019
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

5 participants