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(log): fix panic when log with nil val #3145

Merged
merged 4 commits into from
Jun 7, 2024

Conversation

0xfocus
Copy link
Contributor

@0xfocus 0xfocus commented May 29, 2024

replace Stringer.String with fmt.Sprintf to for log val stringer


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

@0xfocus 0xfocus requested review from a team as code owners May 29, 2024 09:11
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

Thanks @0xfocus ❤️

@melekes melekes added backport-to-v1.x Tell Mergify to backport the PR to v1.x backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x labels May 29, 2024
Copy link
Contributor

@andynog andynog left a comment

Choose a reason for hiding this comment

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

lgtm. Did some diff testing on cometbft log output with the new code and old code, even though the log might not come in the same order, everything seems to be logging the same.

@andynog
Copy link
Contributor

andynog commented May 29, 2024

Thanks @0xfocus, could you please take a look at the lint check?

@0xfocus
Copy link
Contributor Author

0xfocus commented May 30, 2024

Thank you @melekes @andynog , I'm sorry, I am not very familiar with this development process.

Copy link
Contributor

@andynog andynog left a comment

Choose a reason for hiding this comment

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

Could you please provide a changelog entry in this PR? If you check other PRs you will be able to see how to create one in the unreleased folder.

@andynog
Copy link
Contributor

andynog commented May 30, 2024

Thanks @0xfocus, this is ready to merge but noticed there's no changelog entry.

Also, just for additional context, Since there're no issue associated with this, I'd like more context on the issue in particular when you would get a panic for logging a nil val.

Just trying to understand the use case. We might backport this change to older releases but I'm finding interesting the use cases when this panic would be caused. I'd assume that since this piece of code is present even in very older releases, if something would cause a panic when logging I'd assume that this would have surfaced a long time ago. Please let us know. Thanks !

replace `Stringer.String` with `fmt.Sprintf` to for log val stringer
@0xfocus
Copy link
Contributor Author

0xfocus commented May 31, 2024

In my case, I want to log a Hash struct pointer, sometimes the pointer is nil, so the panic happend
list a example

package main

import (
    "encoding/hex"
    "os"

    cmtlog "github.com/cometbft/cometbft/libs/log"
)

type Hash [32]byte

func (h Hash) String() string {
    return "0x" + hex.EncodeToString(h[:])
}

func main() {
    logger := cmtlog.NewTMLogger(cmtlog.NewSyncWriter(os.Stdout))
    var h Hash
    logger.Info("Hash object OK", "hash", h)
    p := &h
    logger.Info("Hash pointer OK", "hash", p)
    p = nil
    logger.Info("nil Hash pointer panic", "hash", p)
}

@melekes melekes removed backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x Tell Mergify to backport the PR to v1.x labels Jun 3, 2024
@melekes melekes requested a review from andynog June 3, 2024 08:11
@0xfocus
Copy link
Contributor Author

0xfocus commented Jun 5, 2024

Hi, @andynog, Is there any problem in this PR?

Copy link
Contributor Author

@0xfocus 0xfocus left a comment

Choose a reason for hiding this comment

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

add changelog entry for #3145

@melekes melekes enabled auto-merge June 7, 2024 08:34
@melekes melekes dismissed andynog’s stale review June 7, 2024 08:42

changelog was added

@melekes melekes added this pull request to the merge queue Jun 7, 2024
Merged via the queue into cometbft:main with commit b13c703 Jun 7, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants