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

Fixes #62 #64

Closed
wants to merge 2 commits into from
Closed

Fixes #62 #64

wants to merge 2 commits into from

Conversation

gneuvill
Copy link
Contributor

@gneuvill gneuvill commented Feb 3, 2017

Of course, there is still http://dilbert.com/strip/2001-10-25 , but anyway com.sun.tools.javac is definitely not thread safe so using parallel streams in annotation processors doesn't seem like a good idea...

Of course, there is still http://dilbert.com/strip/2001-10-25 , but anyway `com.sun.tools.javac` is definitely not thread safe so using parallel streams in annotation processors doesn't seem like a good idea...
@codecov-io
Copy link

codecov-io commented Feb 3, 2017

Codecov Report

Merging #64 into master will not impact coverage.

@@           Coverage Diff            @@
##             master     #64   +/-   ##
========================================
  Coverage      4.93%   4.93%           
- Complexity        0      35   +35     
========================================
  Files            44      44           
  Lines          2208    2208           
  Branches        182     125   -57     
========================================
  Hits            109     109           
  Misses         2094    2094           
  Partials          5       5
Impacted Files Coverage Δ Complexity Δ
...java/org/derive4j/processor/DerivingProcessor.java 0% <ø> (ø) 0 <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1f93e2...54f4376. Read the comment docs.

@jbgi
Copy link
Member

jbgi commented Feb 3, 2017

if java.lang.model is not thread-safe I think it is a jdk bug. but anyway I think the bug is only when loading new jar, and this happen in only one place I think, in DeriveConfigBuilder.findDeriveConfig .
So before we disable parallelization completely, could you try to add a .sequential() just before
.flatMap(e -> deriveConfigBuilder.findDeriveConfig((TypeElement) e)) at this line and see if it fix the problem?

@gneuvill
Copy link
Contributor Author

gneuvill commented Feb 3, 2017

Since the bug occurs at random, I cannot give you a definitive answer (hence the Dilbert reference), but yes, it seems to be an efficient fix...

@jbgi
Copy link
Member

jbgi commented Feb 5, 2017

After some experiments, I went with the strategy of disabling most parallelization (except for the initial discovery of all classes of the round) because there was other places were jar loading could occur.

@jbgi jbgi closed this Feb 5, 2017
@gneuvill
Copy link
Contributor Author

gneuvill commented Feb 5, 2017

I went with the strategy of disabling most parallelization

That seems the wisest... Thanks !

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.

None yet

3 participants