Skip to content

Upgrade to Logback 1.5.7 #41885

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

Closed

Conversation

mches
Copy link
Contributor

@mches mches commented Aug 16, 2024

Upgrades Logback from 1.5.6 to 1.5.7.

The return type of ch.qos.logback.core.ContextBase#getConfigurationLock.getConfigurationLock() has changed from java.lang.Object to java.util.concurrent.locks.ReentrantLock. It's noteworthy that this is a source compatible, binary incompatible change. The change necessitates replacing a synchronized block with a try-finally construct to ensure correct locking semantics going forward. There were also changes to the behavior of ch.qos.logback.classic.LoggerContext#stop() that necessitate calling ch.qos.logback.classic.LoggerContext#reset() where it wasn't already, in tests.

Closes #41887

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 16, 2024
@mches mches force-pushed the feature/upgrade-logback branch from 15f629e to a1c595c Compare August 16, 2024 08:25
@snicoll snicoll changed the title Upgrade Logback to 1.5.7 Upgrade to Logback 1.5.7 Aug 16, 2024
@snicoll
Copy link
Member

snicoll commented Aug 16, 2024

Thanks @mches. I was looking at this and noticed this PR. Our build is broken with just a change of version, namely profiles don't seem to work anymore. I've kicked off a build for this PR to see if your changes make a difference.

@mches
Copy link
Contributor Author

mches commented Aug 16, 2024

@snicoll You're welcome. I had a lot of fun figuring out why it broke. The build works great locally. 🤞🏻

@snicoll snicoll added the type: dependency-upgrade A dependency upgrade label Aug 16, 2024
@snicoll snicoll added this to the 3.3.3 milestone Aug 16, 2024
@snicoll snicoll removed the status: waiting-for-triage An issue we've not yet triaged label Aug 16, 2024
@snicoll snicoll self-assigned this Aug 16, 2024
snicoll pushed a commit that referenced this pull request Aug 16, 2024
snicoll added a commit that referenced this pull request Aug 16, 2024
@snicoll snicoll closed this in 68d600e Aug 16, 2024
@snicoll
Copy link
Member

snicoll commented Aug 16, 2024

Thanks very much @mches. A timely PR indeed!

@wilkinsona
Copy link
Member

Unfortunately, we're going to have to revert this. In Logback 1.5.6 and earlier, stopping the LoggerContext would reset it. In 1.5.7, it only does so if it has been started. We're stopping a context that hasn't been started so reset isn't called. This PR addresses this by calling reset explicitly in some tests, but this doesn't address the issue that the LoggerContext is in the wrong state and may be hiding other bugs that occur due to that. We've seen these in our CI jobs on Windows which fail as the log file is left open and deleting it fails. We need to review how we're managing the lifecycle of the LoggerContext and make sure that it's in the right state. I will use #41887 to do that.

snicoll added a commit that referenced this pull request Aug 16, 2024
This reverts commit 68d600e, reversing
changes made to d4762ec.

See gh-41885
@snicoll snicoll removed this from the 3.3.3 milestone Aug 16, 2024
@snicoll snicoll added the status: declined A suggestion or change that we don't feel we should currently apply label Aug 16, 2024
@mches mches deleted the feature/upgrade-logback branch August 16, 2024 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: dependency-upgrade A dependency upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants