Skip to content

fix concurrent modification exception#4935

Merged
jeremylong merged 1 commit intomainfrom
concurrentModification_removeSingleton
Oct 19, 2022
Merged

fix concurrent modification exception#4935
jeremylong merged 1 commit intomainfrom
concurrentModification_removeSingleton

Conversation

@jeremylong
Copy link
Copy Markdown
Collaborator

Supersedes #4923.

@aikebah this is what I was thinking. currently the persistent object store is on the engine - if it was moved to the settings object there would likely be fewer code changes needed. Thoughts on this approach?

@boring-cyborg boring-cyborg Bot added ant changes to ant core changes to core tests test cases labels Oct 15, 2022
@jeremylong jeremylong requested a review from aikebah October 15, 2022 18:21
@aikebah
Copy link
Copy Markdown
Collaborator

aikebah commented Oct 15, 2022

Supersedes #4923.

@aikebah this is what I was thinking. currently the persistent object store is on the engine - if it was moved to the settings object there would likely be fewer code changes needed. Thoughts on this approach?

Your approach of having it in the engine feels like the clean approach to me. Putting it in settings feels like making the settings object cross the boundary implicitly set by its classname.

@aikebah
Copy link
Copy Markdown
Collaborator

aikebah commented Oct 15, 2022

The significant amount of additional required engine method parameters could be avoided by allowing the analyzers to store a reference to the engine in its AbstractAnalyzer#prepareAnalyzer(Engine engine) implementation.
If done like that the AbstractAnalyzer#analyzeDependency(Settings settings, Engine engine) and AbstractAnalyzer#analyze(Settings settings, Engine engine) could also have the second argument removed.

That refactoring could be done in a separate and later change. To me it feels a bit weird to first request an analyzer to prepare itself for running on behalf of an engine instance and then requiring the same engine instance to be supplied again on each and every analysis.

@jeremylong jeremylong force-pushed the concurrentModification_removeSingleton branch from 7653303 to 4d1c843 Compare October 16, 2022 13:04
@jeremylong jeremylong force-pushed the concurrentModification_removeSingleton branch from 4d1c843 to 7a52353 Compare October 18, 2022 11:41
@jeremylong
Copy link
Copy Markdown
Collaborator Author

@aikebah thank you for the feedback on the PR. The abstract suppression analyzer now stores a reference to the engine and I also moved the call in the engine to report the unused suppression rules to a new analyzer - so that it is cleaner and not a weird one off solution.

@aikebah
Copy link
Copy Markdown
Collaborator

aikebah commented Oct 18, 2022

@jeremylong LGTM apart from the nitpicking review comments of Checkstyle

@jeremylong jeremylong force-pushed the concurrentModification_removeSingleton branch from 7a52353 to edaba8f Compare October 18, 2022 23:25
@jeremylong jeremylong merged commit a4be3be into main Oct 19, 2022
@jeremylong jeremylong deleted the concurrentModification_removeSingleton branch October 19, 2022 10:22
@jeremylong jeremylong added this to the 7.2.2 milestone Oct 19, 2022
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Dec 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

ant changes to ant core changes to core tests test cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants