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

Remove dead panic in Entry.Panic #1212

Merged
merged 1 commit into from Dec 17, 2020
Merged

Conversation

alecbz
Copy link
Contributor

@alecbz alecbz commented Dec 15, 2020

Entry.log itself panics when the log level is set to PanicLevel, (and
PanicLevel is always eneabled) so this second panic will never be reached.


This change is Reviewable

[Entry.log itself panics][0] when the log level is set to PanicLevel, (and
PanicLevel is always eneabled) so this second panic will never be reached.

[0]: https://github.com/sirupsen/logrus/blob/8ae478eb8a850a54ea4915a2b572f377de1f9c7e/entry.go#L253
@alecbz
Copy link
Contributor Author

alecbz commented Dec 16, 2020

@sirupsen

@dgsb
Copy link
Collaborator

dgsb commented Dec 17, 2020

Hello @alecbz thanks for your contribution. However this change would make the code correctness dependant of the Panic specific value. I'm not sure it's a good idea to do that.

@alecbz
Copy link
Contributor Author

alecbz commented Dec 17, 2020

@dgsb Can you elaborate? AFAICT right now this line is never run, so removing it should have no effect. Is there a situation where this line can be reached?

edit: Oh, or are you talking about the test? That test is more-or-less a mirror of the TestEntryPanicf test right above it, except it uses Panic instead of Panicf.

@dgsb
Copy link
Collaborator

dgsb commented Dec 17, 2020

I mean in term of maintainability, if someday we add a new level like

const (
  EarthQuakeLevel = iota
  PanicLevel

This would make the code incorrect and change its behaviour

@alecbz
Copy link
Contributor Author

alecbz commented Dec 17, 2020

Well, first of all, Panicf doesn't have this direct panic call, so if the idea is that this panic is intentionally there, it should probably be added to Panicf as well. It also panics a different value than the panic inside Entry.log (panics just the string message, instead of the log entry), so it seems like that should also be made consistent.

But more to the point, what would you say the intended behavior should be if someone calls Panic on an EarthQuakeLevel logger? I'd imagine no-oping would be the correct approach, in which case the code as-is would be incorrect if a new EarthQuake level were added.

If the idea is that we want to special-case Panic/Panicf so that it always causes a panic even when used on a hypothetical EarthQuakeLevel logger, then we should probably add code to Panicf to also panic directly (and remove the inner panic from Entry.log, since it would be redundant).

@dgsb
Copy link
Collaborator

dgsb commented Dec 17, 2020

Ok, let's merge that.

Regarding hypothetical new level and side effect behaviour, you may want tool look at what the Fatal calls do.
It should exit the process whatever the current configured level of the logger is. This is what is expected by the user.

As a personal note, I'd rather not have level at all which trigger such side effects:

  • Adding an explicit os.Exit in caller is not that hard and would keep the code base of logrus cleaner and simpler.
  • Fatal level call should not used quite often except at application startup

@dgsb dgsb merged commit cd4bf4e into sirupsen:master Dec 17, 2020
@alecbz
Copy link
Contributor Author

alecbz commented Dec 17, 2020

look at what the Fatal calls do. It should exit the process whatever the current configured level of the logger is. This is what is expected by the user.

Ah yeah, the code there correctly calls entry.Logger.Exit(1) in each of Fatal, Fatalf, and Fatalln.

Forcing a panic in all situations doesn't sound unreasonable, but yeah, if that's the direction we want to go, we should add the call to each of Panic, Panicf, and Panicln, and remove it from the inner Entry.log.

@alecbz alecbz deleted the alec/remove-dead-panic branch December 17, 2020 16:37
mikewotton added a commit to NandosUK/logrus that referenced this pull request Feb 12, 2021
…-panic"

This reverts commit cd4bf4e, reversing
changes made to 44d983d.
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.

None yet

2 participants