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

Raise log level when bean destruction fails #23200

Closed
wants to merge 2 commits into from
Closed

Raise log level when bean destruction fails #23200

wants to merge 2 commits into from

Conversation

boojongmin
Copy link
Contributor

@boojongmin boojongmin commented Jun 27, 2019

related to this issue #23199

logger runned on org.springframework.beans.factory.support package.
but InvocationTargetException was makes by my code.
I want to check my error log.

@sbrannen sbrannen requested a review from jhoeller June 27, 2019 10:11
@sbrannen sbrannen added the in: core Issues in core modules (aop, beans, core, context, expression) label Jun 27, 2019
@boojongmin boojongmin changed the title modify log level info to warn when destroy bean modify log level info to ERROR when destroy bean Jun 27, 2019
@sbrannen sbrannen changed the title modify log level info to ERROR when destroy bean Raise log level to ERROR when bean destruction fails Jul 1, 2019
@jhoeller jhoeller added this to the 5.1.9 milestone Jul 1, 2019
@jhoeller jhoeller added type: enhancement A general enhancement type: regression A bug that is also a regression and removed for: team-attention status: waiting-for-triage An issue we've not yet triaged or decided on type: enhancement A general enhancement labels Jul 1, 2019
@jhoeller jhoeller self-assigned this Jul 1, 2019
@jhoeller
Copy link
Contributor

jhoeller commented Jul 3, 2019

On review, this is a regression from 5.0/4.3 where we mostly logged bean destruction exceptions at warn level. Restoring this in 5.1.9 seems feasible, even within the revised log level usage that we introduced in 5.1 (for less noise at info and warn level). That said, such exceptions are technically not a hard error but rather just a partial effect in a multi-step destruction process, so logging at error level would arguably create unnecessary (follow-up) noise for scenarios with many related beans that need to be destroyed. This is particularly relevant for production scenarios where error level may trigger operator escalations.

Generally speaking, we recommend running with info or at least warn log level in development. Also, destroy method ideally should not propagate exceptions but rather handle them locally (possibly even log them locally with a descriptive log message).

@jhoeller jhoeller changed the title Raise log level to ERROR when bean destruction fails Raise log level when bean destruction fails Jul 3, 2019
@boojongmin
Copy link
Contributor Author

@jhoeller

Thank you for your kind reply.
Kotlin handles errors with an unchecked exception.
My mistake is that when I close a resource like db, I miss the try-catch block related to checked exception like Java. I think kotlin programmer might have experience like me when I close resources.
Thank you.

@jhoeller jhoeller closed this in 56cc0d0 Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: regression A bug that is also a regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants