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

Suppress exceptions from toString methods #22

Merged
merged 2 commits into from
Jun 19, 2017
Merged

Conversation

imanushin
Copy link
Contributor

Sometimes classes have incorrect toString implementation, which raises exception.

To avoid this, logger should cautch and inform developer.

Such behavior is used in the many popular loggers, we should reuse it too

@oshai
Copy link
Owner

oshai commented Jun 19, 2017

Can you give an example of popular loggers frameworks that catch the exception? it seems like developers will lose information this way. Also, why not catch it at the user (of this lib) code?

@imanushin
Copy link
Contributor Author

@oshai , code which does formatting (invoke function in our case) should catch exception.

Examples:

        try {
            final MessageFormat format = new MessageFormat(msgPattern);
            final Format[] formats = format.getFormats();
            if (formats != null && formats.length > 0) {
                return new MessageFormatMessage(locale, msgPattern, args);
            }
        } catch (final Exception ignored) {
            // Obviously, the message is not a proper pattern for MessageFormat.
        }
        try {
            if (MSG_PATTERN.matcher(msgPattern).find()) {
                return new StringFormattedMessage(locale, msgPattern, args);
            }
        } catch (final Exception ignored) {
            // Also not properly formatted.
        }

Ideally: developer should not get any exceptions from logger. There are several sets of errors:

  • Formatting errors - should be caught by us, because we do the formatting
  • Stream Writing errors - is done by downstream logger
  • Global: OOM, StackOverflow, etc. - should be skipped

@oshai
Copy link
Owner

oshai commented Jun 19, 2017

ok, thanks. I will merge with some fixes to message itself.

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.

2 participants