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

Compilation failure with parallel builds #183

Closed
joeshannon opened this issue Jul 12, 2021 · 23 comments · Fixed by #191
Closed

Compilation failure with parallel builds #183

joeshannon opened this issue Jul 12, 2021 · 23 comments · Fixed by #191
Milestone

Comments

@joeshannon
Copy link
Contributor

@joeshannon joeshannon commented Jul 12, 2021

Testing the staged 2.4.0 release and seeing issues with parallel builds:

[ERROR] /scratch/30day_tmp/delete12jul/tycho-issue/xtext-eclipse/org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/refactoring2/rename/DefaultLinkedPositionGroupCalculator2.java: 
[ERROR] 	/**
[ERROR] 	^
[ERROR] Internal compiler error: java.nio.file.ClosedFileSystemException at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.ensureOpen(ZipFileSystem.java:1146)
[ERROR] java.nio.file.ClosedFileSystemException
[ERROR] 	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.ensureOpen(ZipFileSystem.java:1146)
[ERROR] 	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.checkAccess(ZipFileSystem.java:362)
[ERROR] 	at jdk.zipfs/jdk.nio.zipfs.ZipPath.checkAccess(ZipPath.java:843)
[ERROR] 	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystemProvider.checkAccess(ZipFileSystemProvider.java:185)
[ERROR] 	at java.base/java.nio.file.Files.exists(Files.java:2440)
[ERROR] 	at org.eclipse.jdt.internal.compiler.batch.ClasspathJep247.findClass(ClasspathJep247.java:84)
[ERROR] 	at org.eclipse.jdt.internal.compiler.batch.FileSystem.internalFindClass(FileSystem.java:472)
[ERROR] 	at org.eclipse.jdt.internal.compiler.batch.FileSystem.findClass(FileSystem.java:414)
[ERROR] 	at org.eclipse.jdt.internal.compiler.batch.FileSystem.findType(FileSystem.java:511)
[ERROR] 	at org.eclipse.jdt.internal.compiler.env.IModuleAwareNameEnvironment.findType(IModuleAwareNameEnvironment.java:97)
[ERROR] 	at org.eclipse.jdt.internal.compiler.lookup.LookupEnvironment.askForType(LookupEnvironment.java:239)
[ERROR] 	at org.eclipse.jdt.internal.compiler.lookup.LookupEnvironment.getType(LookupEnvironment.java:1732)
[ERROR] 	at org.eclipse.jdt.internal.compiler.lookup.LookupEnvironment.getResolvedType(LookupEnvironment.java:1662)
[ERROR] 	at org.eclipse.jdt.internal.compiler.lookup.LookupEnvironment.getResolvedJavaBaseType(LookupEnvironment.java:1674)
[ERROR] 	at org.eclipse.jdt.internal.compiler.lookup.Scope.getJavaLangInvokeMethodHandlesLookup(Scope.java:2941)
[ERROR] 	at org.eclipse.jdt.internal.compiler.ClassFile.generateBootstrapMethods(ClassFile.java:3614)
[ERROR] 	at org.eclipse.jdt.internal.compiler.ClassFile.addAttributes(ClassFile.java:436)
[ERROR] 	at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:772)
[ERROR] 	at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:831)
[ERROR] 	at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.generateCode(CompilationUnitDeclaration.java:412)
[ERROR] 	at org.eclipse.jdt.internal.compiler.Compiler.process(Compiler.java:913)
[ERROR] 	at org.eclipse.jdt.internal.compiler.ProcessTaskManager.run(ProcessTaskManager.java:145)
[ERROR] 	at java.base/java.lang.Thread.run(Thread.java:829)

Initially observed with my company's build. To reproduce here I used the Xtext build (just because I saw it referenced in a recent ticket) with mvn -T2 -DskipTests -Platest clean verify after switching the "latest" profile to use 2.4.0.

I attempted to reproduce with a minimal project but couldn't - I think because the dependency tree was too linear.

@laeubi
Copy link
Contributor

@laeubi laeubi commented Jul 13, 2021

From the stacktrace this seems to be related to JDT instead. Don't know if JDT Batch compiler is maybe not threadsafe (e.g. using global static state) maybe @mickaelistria can tell more here?

@joeshannon
Copy link
Contributor Author

@joeshannon joeshannon commented Jul 13, 2021

Yes I think you are right: https://bugs.eclipse.org/bugs/show_bug.cgi?id=574450
I should have checked the JDT bugs first but I suppose it's good to be aware that this might cause issues with the imminent release.

We can probably close this now? Or would it be helpful to keep a Tycho issue open until JDT is fixed (and updated here).

@mickaelistria
Copy link
Contributor

@mickaelistria mickaelistria commented Jul 13, 2021

I don't think JDT compiler is to be assumed threadsafe.
Merging 6895ce4 in #27 (by @jhonnen ) was a bit too optimistic. We need to restore the synchronization lock around the compile section.

@jhonnen
Copy link
Contributor

@jhonnen jhonnen commented Jul 13, 2021

Do you now where the shared state comes from? I can't tell from the stacktrace, ClasspathJep247 seems to be instantiated non-static per invocation.

If we do need to restore the lock, can we add an opt-out mechanism?

@mickaelistria
Copy link
Contributor

@mickaelistria mickaelistria commented Jul 13, 2021

See last comment by @joeshannon about https://bugs.eclipse.org/bugs/show_bug.cgi?id=574450 being the root cause.

If we do need to restore the lock, can we add an opt-out mechanism?

I think we do need to restore the lock because we don't want people to be stuck against older releases because this bug blocks them; and restoring the lock wouldn't constitute a regression compared to current 2.3 release.
The code should reference clearly https://bugs.eclipse.org/bugs/show_bug.cgi?id=574450 so we know we should be able to remove the lock whenever this upstream issue is fixed.
A flag to synchronize or not (synchronized by default, parallel is flag is set) would be fine; as long as the flag is not advertized as something that is sustainable to rely one as we'll remove it as soon as https://bugs.eclipse.org/bugs/show_bug.cgi?id=574450 is fixed.

@jhonnen
Copy link
Contributor

@jhonnen jhonnen commented Jul 13, 2021

The code should reference clearly https://bugs.eclipse.org/bugs/show_bug.cgi?id=574450 so we know we should be able to remove the lock whenever this upstream issue is fixed.

If we only want to work around this particular bug, we could also use synchronization only if compiled with --release.

@mickaelistria
Copy link
Contributor

@mickaelistria mickaelistria commented Jul 13, 2021

If we only want to work around this particular bug, we could also use synchronization only if compiled with --release.

whatever works ;)

@jhonnen
Copy link
Contributor

@jhonnen jhonnen commented Jul 13, 2021

OK, I'll submit a PR tomorrow.

@jhonnen
Copy link
Contributor

@jhonnen jhonnen commented Jul 14, 2021

Seems like Andrey was faster and the JDT bug is already fixed. Would that workaround be part of tycho 2.4.0 or will there be another tycho release before the 2021-09 release?

@laeubi
Copy link
Contributor

@laeubi laeubi commented Jul 14, 2021

@akurtakov can we use the SNAPSHOT Build from JDT that includes the patch?

@akurtakov
Copy link
Contributor

@akurtakov akurtakov commented Jul 14, 2021

Tycho uses https://mvnrepository.com/artifact/org.eclipse.jdt/ecj and until JDT releases new artifact to maven central the answer is no.

@akurtakov
Copy link
Contributor

@akurtakov akurtakov commented Jul 14, 2021

Tycho provides a way to override jdt version used (see https://git.eclipse.org/c/platform/eclipse.platform.releng.aggregator.git/tree/eclipse-platform-parent/pom.xml#n429 ) so this can be done if one provides own way to get whatever jdt version he wants like platform does.

@laeubi
Copy link
Contributor

@laeubi laeubi commented Jul 14, 2021

until JDT releases new artifact to maven central

I wonder if it would be possible if JDT releases snapshots to the eclipse-maven server like tycho does? I'm not familiar with this deploy stuff and eclipse-infra but it seems that such a thing would ease much things (e.g. tycho snapshot can always use JDT snapshots to find such problems early before the release).

@akurtakov
Copy link
Contributor

@akurtakov akurtakov commented Jul 14, 2021

Platform releng hat on: JDT snapshots are not autodeployed - it's done manually for milestones and on request by jdt developers for critical fixes. Automating publishing to eclipse maven repo would be nice but even in platform we have never considered using nightly ecj for our nightly builds - it just sounds too risky. Not to mention the fact that changing ecj versions causes comparator issues due to unstable ordering of generated lambdas ( [https://bugs.eclipse.org/bugs/show_bug.cgi?id=561135]).

Tycho hat on: Switching tycho snapshots to use JDT snapshot (milestone, critical fix) as used by platform sounds good approach to me if we find a way to compile and run our tests with this snapshot but users of tycho snapshot should still use released jdt version to not decrease general usability of tycho snapshots for the general public.

@laeubi
Copy link
Contributor

@laeubi laeubi commented Jul 14, 2021

Automating publishing to eclipse maven repo would be nice but even in platform we have never considered using nightly ecj for our nightly builds - it just sounds too risky.

Can you trigger/ask/investigate this? I think automatic workflows are always better than manual tasks. Beside that, would this bug qualify as one to make a manual deploy feasible so tycho can use that then?

if we find a way to compile and run our tests with this snapshot but users of tycho snapshot

If I remember right @mickaelistria has setup a build that runs tycho with latest maven-snapshot, maybe something similar would be possible for ecj as well?

@akurtakov
Copy link
Contributor

@akurtakov akurtakov commented Jul 14, 2021

Automating publishing to eclipse maven repo would be nice but even in platform we have never considered using nightly ecj for our nightly builds - it just sounds too risky.

Can you trigger/ask/investigate this? I think automatic workflows are always better than manual tasks. Beside that, would this bug qualify as one to make a manual deploy feasible so tycho can use that then?

When I say manual I mean deployment is triggered manually but it's jenkins job. It's deployed here so tycho can override to use latest from here. I don't think there is more needed or even wanted to be done from platform side.

if we find a way to compile and run our tests with this snapshot but users of tycho snapshot

If I remember right @mickaelistria has setup a build that runs tycho with latest maven-snapshot, maybe something similar would be possible for ecj as well?

That shouldn't be too hard to setup.

@sravanlakkimsetti
Copy link

@sravanlakkimsetti sravanlakkimsetti commented Jul 14, 2021

Deployed new ecj as

<groupId>org.eclipse.jdt</groupId>
<artifactId>ecj</artifactId>
<version>3.27.0.v20210713-2148</version>

repository:
https://repo.eclipse.org/content/repositories/eclipse-staging/

@joeshannon
Copy link
Contributor Author

@joeshannon joeshannon commented Jul 15, 2021

I can confirm also that this ecj version resolves the issue with the builds I was originally testing with.

@mickaelistria
Copy link
Contributor

@mickaelistria mickaelistria commented Jul 19, 2021

@jhonnen Are you planning to submit a PR today? If not, I'll take care of it.

@jhonnen
Copy link
Contributor

@jhonnen jhonnen commented Jul 19, 2021

I wasn't sure if we still need this change in tycho with the fixed ECJ.

I have no time for this today, but I'll can submit one first thing tomorrow.

@mickaelistria
Copy link
Contributor

@mickaelistria mickaelistria commented Jul 19, 2021

Tycho usually uses the latest release of ECJ. The provided build is helpful for people who feel comfortable hacking their pom, but not really to use by default.

mickaelistria added a commit to mickaelistria/tycho that referenced this issue Jul 19, 2021
Compiler cannot use parallel execution until related bugs is fixed in
the version used and provided by default in Tycho.
@mickaelistria mickaelistria linked a pull request that will close this issue Jul 19, 2021
@mickaelistria
Copy link
Contributor

@mickaelistria mickaelistria commented Jul 19, 2021

Submitted #191

mickaelistria added a commit to mickaelistria/tycho that referenced this issue Jul 19, 2021
Compiler cannot use parallel execution until related bugs is fixed in
the version used and provided by default in Tycho.
mickaelistria added a commit that referenced this issue Jul 19, 2021
Compiler cannot use parallel execution until related bugs is fixed in
the version used and provided by default in Tycho.
mickaelistria added a commit that referenced this issue Jul 19, 2021
Compiler cannot use parallel execution until related bugs is fixed in
the version used and provided by default in Tycho.
@mickaelistria
Copy link
Contributor

@mickaelistria mickaelistria commented Jul 19, 2021

Please try https://oss.sonatype.org/content/repositories/orgeclipsetycho-1068 as new staging repo for 2.4 release, which should include the fix. I hope to release it tomorrow afternoon unless issue is not resolved or other blockers are found.

@laeubi laeubi added this to the 2.5 milestone Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants