fix: lower native library candidate log level from WARNING to DEBUG (fixes #2185) - #2192
Conversation
…ixes #2185) loadNativeLibrary() is called as part of a fallback chain that tries multiple candidate paths. A failed candidate is expected and recovered from by the next step, so WARNING is too noisy — the genuinely unrecoverable case already throws from loadJLineNativeLibrary(). The WARNING also causes a practical problem: applications that bridge System.Logger into terminal-backed logging re-enter initialize() through the static synchronized lock, deadlocking every thread that tries to build a terminal. Lowering to DEBUG keeps the diagnostic available when needed without triggering application-level logging on every recovered candidate.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe native loader now logs failed native library loads at DEBUG level instead of WARNING level. The related documentation now states the DEBUG-level behavior. The method still returns ChangesNative load logging
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The change lowers logging for recoverable native-library candidates without changing the fallback behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
✅ LGTM — Clean, minimal fix that correctly lowers a log level from WARNING to DEBUG for expected/recovered native library candidate failures.
The change is correct:
loadNativeLibrary()is called as part of a fallback chain — failure of any single candidate is expected and recovered by the next candidate. WARNING is inappropriate for a recovered condition.- No diagnostic information is lost: the unrecoverable case throws with all
triedPathsjoined, providing full context when all candidates fail. - The deadlock scenario is real and well-documented in #2185 —
System.LoggerWARNING bridged to application logging that re-entersstatic synchronized initialize(). - The Javadoc update ("logs the error at DEBUG level") is consistent with the code change.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Labels | (none) | bug |
| Milestone | (none) | 4.4.0 |
🔀 Backport Status
master but no backport was found for:
4.0.x— has the identical WARNING at the same location. Since this fixes a deadlock affecting real users (Maven), a cherry-pick should be straightforward.jline-3.x— may also warrant a backport but uses a different logging mechanism and would need adaptation.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
1 participant
Summary
loadNativeLibrary()fromWARNINGtoDEBUGfor failed native library candidates that the fallback chain recovers fromloadJLineNativeLibrary()with fulltriedPaths, so no diagnostic information is loststatic synchronized initialize()through application logging bridges that render to the terminal being builtContext
When
library.jline.pathis set (e.g. Maven sets it to${maven.home}/lib/jline-native), a failure of the first candidate warns even though the next step succeeds. TheSystem.LoggerWARNING is bridged by applications into their own logging, and if that logging renders through the terminal currently being constructed, the thread parks inside thestatic synchronized initialize()lock — blocking every other terminal build in the JVM.Reported in #2185, observed in Maven (apache/maven#12814, apache/maven-executor#38).
Summary by CodeRabbit