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

#858 Fix child module resolution for multi-level projects #859

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

corebonts
Copy link

Child modules were processed in a parallel stream which added elements to an unsynchonized LinkedHashMap.

This may resulted an incomplete set of child modules.

@corebonts corebonts force-pushed the fix858 branch 2 times, most recently from 3e1ebcf to 9bf94d8 Compare December 16, 2022 15:41
@andrzejj0
Copy link
Contributor

@corebonts please also modify https://github.com/mojohaus/versions/blob/master/versions-maven-plugin/src/main/java/org/codehaus/mojo/versions/SetMojo.java#L375

Modules were processed in a parallel stream which added elements to
an unsynchonized LinkedHashMap / LinkedHashSet.

This may resulted an incomplete set of child modules.

Note that as the stream items are not processed heavily there is no or minor
advantage of parallel processing.
@andrzejj0
Copy link
Contributor

Closed my own PR fixing this. @slachiewicz @slawekjaranowski could you please approve the run? Thanks

@slawekjaranowski
Copy link
Member

slawekjaranowski commented Dec 16, 2022

I also see that parallelStream is used in many other places

I don't see a much performance benefit of used it but can cause a problem like here.
Maybe we simply can replace all occurrences.

Can be done is next PR 😄

@andrzejj0
Copy link
Contributor

andrzejj0 commented Dec 16, 2022

Went other these during the evaluation of this one. Basically a problem occurs when there is a modification of a non-thread safe collection during stream execution. There should be no problem with the creation of a new collection, which is the case in all other places than the corrected in this PR.

Having said that, replacing that with a simple stream would be a conservative take and definitely on the safe side. Experience shows that I can be wrong :)

@corebonts
Copy link
Author

After Andrzej told me about the other problematic class, I also went through all the other usages and I can confirm that the others are ok for now. But I neither see a big performance benefit of using parallel streams.

@slawekjaranowski slawekjaranowski merged commit 6619919 into mojohaus:master Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Versions Maven Plugin updates random Module subsets with 2.14.1
4 participants