Skip to content
This repository was archived by the owner on Mar 18, 2026. It is now read-only.

Fix multithreading issues and provide parse exceptions#45

Merged
aaronhurst-google merged 7 commits into
google:mainfrom
adangel:parse-errors
Nov 11, 2023
Merged

Fix multithreading issues and provide parse exceptions#45
aaronhurst-google merged 7 commits into
google:mainfrom
adangel:parse-errors

Conversation

@adangel

@adangel adangel commented Nov 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes part of pmd/pmd#4722

The strange errors in Dao.cls and Dao_test.cls should be gone now (The files can actually be parsed completely fine).

PMD analyzes the project in multiple threads and the node counter in com.google.summit.ast.Node was not thread safe, leading to errors like

[PmdThread 1] WARN com.google.summit.SummitAST - Failed to translate <str>
com.google.summit.translation.Translate$TranslationException: Number of created nodes 92 should match number of reachable nodes 69
	at com.google.summit.translation.Translate.translate(Translate.kt:126)
	at com.google.summit.SummitAST.parseAndTranslate$_maven_lib(SummitAST.kt:133)
	at com.google.summit.SummitAST.parseAndTranslate(SummitAST.kt:91)
	at net.sourceforge.pmd.lang.apex.ast.ApexParser.parse(ApexParser.java:31)
	at net.sourceforge.pmd.lang.apex.ast.ApexParser.parse(ApexParser.java:19)
	at net.sourceforge.pmd.lang.impl.PmdRunnable.parse(PmdRunnable.java:112)
	at net.sourceforge.pmd.lang.impl.PmdRunnable.processSource(PmdRunnable.java:132)
	at net.sourceforge.pmd.lang.impl.PmdRunnable.run(PmdRunnable.java:80)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)

This also propagates the exceptions down to PMD, so that we can deal with the exception rather than checking whether the parsed AST is null.

  • Tests pass
  • Appropriate changes to README are included in PR

@adangel
Set version to 2.1.1-SNAPSHOT
8d294dc
@adangel
Provide better error messages, throw ParseException
b27ae76
@adangel
Use thread local for node count
b1a9e1b 
Otherwise, the translation process is not thread safe, as Node might
be created by multiple threads.
This was referenced Nov 5, 2023
Closed
Merged
aaronhurst-google
aaronhurst-google approved these changes Nov 11, 2023
View reviewed changes

@aaronhurst-google aaronhurst-google left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@aaronhurst-google aaronhurst-google merged commit 729092c into google:main Nov 11, 2023
@adangel adangel deleted the parse-errors branch November 23, 2023 16:41
@adangel

adangel commented Nov 23, 2023

Copy link
Copy Markdown
Contributor Author

@aaronhurst-google Thanks for merging! When do you think you can create a new release (2.1.1) which is available in maven central?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@aaronhurst-google aaronhurst-google aaronhurst-google approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@adangel @aaronhurst-google