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

XWIKI-20546: Log CSS exception details #3518

Merged
merged 2 commits into from
Sep 26, 2024

Conversation

amottier
Copy link
Contributor

Jira URL

https://jira.xwiki.org/browse/XWIKI-20546

Changes

Description

  • In XWikiWikiModel, if parsing CSS exception occurs, includes the exception in SLF4J call to get the full stack trace in the logs. This should help investigation of XWIKI-20546 issue.

Clarifications

  • Root cause of the issue might be related to thread safety issue with CSSOMParser.

Executed Tests

mvn -f xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki/pom.xml clean install with Java 17 in Linux environment.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches: No

In XWikiWikiModel, if parsing CSS exception occurs include the exception in SLF4J call to get the full stack trace.
This should help investigation of [XWIKI-20546](https://jira.xwiki.org/browse/XWIKI-20546) issue.
@@ -312,7 +312,7 @@ private String getImageDimension(String dimension, Map<String, String> imagePara
value = sd.getPropertyValue(dimension);
} catch (Exception e) {
// Ignore the style parameter but log a warning to let the user know.
this.logger.warn("Failed to parse CSS style [{}]", style);
this.logger.warn("Failed to parse CSS style [{}]", style, e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We tend to only log a org.apache.commons.lang3.exception.ExceptionUtils.getRootCauseMessage(e) in the case of a warning (a printed exception is pretty huge in the log, so we do that only for errors).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tmortagne for clarifying this coding rule.

Actually what I'm might do is deploying this custom version only in our development environment, reproduce the issue described in XWIKI-20546 and add the log in the Jira ticket comments.

I'm closing this pull request. Thanks for taking the time to look at it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I feel that adding at least the root cause message would be an improvement in any case, it's definitely not great to not have any kind of clue of what was wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reopening the pull request this time logging only the root cause.

Also I took the time to read https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/ and properly configure my IDE.

@amottier amottier closed this Sep 26, 2024
In XWikiWikiModel, if parsing CSS exception occurs include the root cause of the exception in the log message.
This should help investigation of [XWIKI-20546](https://jira.xwiki.org/browse/XWIKI-20546) issue.
@amottier amottier reopened this Sep 26, 2024
@tmortagne tmortagne merged commit 4556c09 into xwiki:master Sep 26, 2024
2 checks passed
@tmortagne
Copy link
Member

tmortagne commented Sep 26, 2024

Thanks a lot for your time @amottier !

That being said, my understanding is that it's not really a fix for https://jira.xwiki.org/browse/XWIKI-20546 and that we need to fix what cause this failure in the first place before closing the issue, right ?

@amottier amottier deleted the log-css-parsing-exception branch September 26, 2024 19:49
@amottier
Copy link
Contributor Author

Yes you are totally right @tmortagne, main purpose of this pull request is to get appropriate level of information to be able to further analyze the issue.

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.

3 participants