modifying exception handling for completeSetup, now it will print the log and stack traces together#2151
modifying exception handling for completeSetup, now it will print the log and stack traces together#21514o4E wants to merge 1 commit intoBentoBoxWorld:developfrom
Conversation
… log and stack traces together
|
Null-pointer in Biomes is not relevant to the issue. What exactly happened before: |
tastybento
added
the
Type: Enhancement
not npe in biome, look at this exception, that's what caused the problem, looks like other plugins are registering permissions at the same time btw, the previous implementation, will first execute |
There was a problem hiding this comment.
Pull request overview
This PR updates BentoBox’s completeSetup exception handling so that the initial failure message and the associated stack trace are emitted as a single logger event (reducing log interleaving), then triggers a shutdown/disable flow.
Changes:
- Replace
fireCriticalError(...); e.printStackTrace();with aLogger#log(Level, message, throwable)call to print message + stack trace together. - Explicitly set
shutdown = true, disable addons, and disable the plugin whencompleteSetupthrows.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (Exception e) { | ||
| fireCriticalError(e.getMessage(), ""); | ||
| e.printStackTrace(); | ||
| getLogger().log(Level.WARNING, |
There was a problem hiding this comment.
This exception path disables the plugin, so logging it at WARNING is likely too low severity and inconsistent with other critical failure paths that use logError()/SEVERE. Consider using Level.SEVERE here (or delegating to a helper that logs at SEVERE with the Throwable).
| getLogger().log(Level.WARNING, | |
| getLogger().log(Level.SEVERE, |
| // Do not save players or islands, just shutdown | ||
| shutdown = true; | ||
| // Stop all addons | ||
| if (addonsManager != null) { | ||
| addonsManager.disableAddons(); | ||
| } | ||
| instance.setEnabled(false); |
There was a problem hiding this comment.
The shutdown/disable sequence here duplicates the logic in fireCriticalError(...) (shutdown flag, disabling addons, disabling the plugin). To reduce divergence risk, consider refactoring to reuse the existing method (e.g., extend fireCriticalError to accept a Throwable for combined message+stack trace, then call it from here).
| fireCriticalError(e.getMessage(), ""); | ||
| e.printStackTrace(); | ||
| getLogger().log(Level.WARNING, | ||
| "unexpected exception occurred during completeSetup, Disabling BentoBox...", e); |
There was a problem hiding this comment.
Log message casing/grammar: consider capitalizing the first word ("Unexpected") and avoiding the comma splice (e.g., split into two sentences) for consistency with other BentoBox log lines.
| "unexpected exception occurred during completeSetup, Disabling BentoBox...", e); | |
| "Unexpected exception occurred during completeSetup. Disabling BentoBox...", e); |
Before this PR, when an exception occurred during a
completeSetuplike this, the log would look like the following, with CRITICAL ERROR and stack trace not connected together.After this PR, the logs will be as follows
Regarding the reason for this exception, I suspect that other plugins have run asynchronous tasks to modify permissions, but further testing is needed to determine the cause. Currently, this exception can be replicated multiple times on my server.
Perhaps it is possible to advance permission registration to
onEnable?