2 points: 1- "Calling System.gc sometimes works." so I'd remove all these gc() which are undeterministic fixes - which is worse than a known bug to me 2- anyway don't forget to fix PMD issues created by this kind of commit "PMD Failure: org.apache.openejb.loader.Files:175 Rule:DoNotCallGarbageCollectionExplicitly Priority:2 Do not explicitly trigger a garbage collection.."
Romain Manni-Bucau @rmannibucau <https://twitter.com/rmannibucau> | Blog <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> | LinkedIn <https://www.linkedin.com/in/rmannibucau> | Tomitriber <http://www.tomitribe.com> 2015-03-16 8:57 GMT+01:00 Romain Manni-Bucau <[email protected]>: > Was not about perf but code readability and maintainance. Already removed > several gc calls in our code cause they were not justified and not a fan of > such workaround - which are not even guaranteed to work. > > Can we use native to fix it? Would maybe be more sure no? > Le 16 mars 2015 08:29, "Andy Gumbrecht" <[email protected]> a > écrit : > > Yes JL, not sure why I missed one? I will fix tonight. >> >> No Romain, just reverting will not fix the issue at all as the gc call is >> a >> known workaround for Win only and there will never be a fix for this. It >> is >> a known Windows IO issue for delete, mkdirs and mkdir in the OS - >> JDK-4715154 >> is just one of the first and clear references, but there are many more >> related issues that focus on the buffers that are not flushed until gc is >> called - Feel free to also search for similar issues if you have time. The >> gc call is only made on the broken OS so it will not affect other systems >> - >> Telling customers to switch to another OS or runtime is not impossible, >> but >> you can try if you like ;-) - I'll send you some 50 or so company >> addresses >> to get you started. There was no previous fix? If what you are referring >> to >> is the use of different files in the test, well that is not a fix, that's >> a >> one time workaround. >> >> Yes Mark, as mentioned on IRC I have seen this before on Win platforms. >> Not >> sure how many times 'Files.remove' etc. is actually called during runtime, >> but it's impact is minimal. The test now passes consistently using the >> same >> file on Win platforms. I tested on Win7+8 using JDK 6+7+8 just to be sure. >> I was able to reproduce on Win7+8 on JDK6+8 as is, but writing just a >> simple del+create+file loop test will hit this on any jdk/win combination >> pretty consistently - the gc eliminates it. >> >> And this topic just forced me to bench it just now. In 100 cycles the gc >> adds +- <10ms on my system per run when compared to using 100 new files >> (ie. the gc call using the same file name is sometimes actually faster >> than >> creating new 100 files). >> >> Andy. >> >> On 15 March 2015 at 19:55, Mark Struberg <[email protected]> wrote: >> >> > >> > - File file = new File("target/test/foo2.jar"); >> > >> > + final File file = new File("target/test/foo.jar"); >> > >> > Andy, did you also look at the Resolution of JDK-4715154? >> > It will not be fixed. As far as I've seen it (only?) happens if you have >> > WindowsDefender running. >> > >> > I'm not quite sure if the gc() call really helps in all situations, but >> > using different files did. >> > >> > LieGrue, >> > strub >> > >> > >> > >> > >> > >> > > On Thursday, 12 March 2015, 14:58, Romain Manni-Bucau < >> > [email protected]> wrote: >> > > > Id revert it using a fixed jvm - we shoulfnt rely on gc at all + you >> > > reverted fixes from Mark so please correct it. >> > > Le 12 mars 2015 11:36, "Jean-Louis Monteiro" >> > > <[email protected]> a >> > > écrit : >> > > >> > > >> > >> In case you missed it, there is one System.gc() not in the if >> > condition. >> > >> As System.gc() should in theory never been called, I would try to >> > minimize >> > >> the number of calls (performance, memory, etc side effects). >> > >> >> > >> What do you think? >> > >> >> > >> -- >> > >> Jean-Louis Monteiro >> > >> http://twitter.com/jlouismonteiro >> > >> http://www.tomitribe.com >> > >> >> > >> On Thu, Mar 12, 2015 at 10:07 AM, <[email protected]> wrote: >> > >> >> > >> > Repository: tomee >> > >> > Updated Branches: >> > >> > refs/heads/master 53c704852 -> bda2f2767 >> > >> > >> > >> > >> > >> > Workaround for Windows bug JDK-4715154 >> > >> > >> > >> > >> > >> > Project: http://git-wip-us.apache.org/repos/asf/tomee/repo >> > >> > Commit: >> http://git-wip-us.apache.org/repos/asf/tomee/commit/bda2f276 >> > >> > Tree: http://git-wip-us.apache.org/repos/asf/tomee/tree/bda2f276 >> > >> > Diff: http://git-wip-us.apache.org/repos/asf/tomee/diff/bda2f276 >> > >> > >> > >> > Branch: refs/heads/master >> > >> > Commit: bda2f276764926022db730c5d145a7d5add450b0 >> > >> > Parents: 53c7048 >> > >> > Author: [email protected] <[email protected]> >> > >> > Authored: Thu Mar 12 10:07:25 2015 +0100 >> > >> > Committer: [email protected] <[email protected]> >> > >> > Committed: Thu Mar 12 10:07:42 2015 +0100 >> > >> > >> > >> > >> > ---------------------------------------------------------------------- >> > >> > .../openejb/loader/BasicURLClassPath.java | 2 +- >> > >> > .../java/org/apache/openejb/loader/Files.java | 41 >> > >> +++++++++++++++++++- >> > >> > .../loader/provisining/MavenResolverTest.java | 12 +++--- >> > >> > 3 files changed, 47 insertions(+), 8 deletions(-) >> > >> > >> > ---------------------------------------------------------------------- >> > >> > >> > >> > >> > >> > >> > >> > >> > >> >> > > >> > >> http://git-wip-us.apache.org/repos/asf/tomee/blob/bda2f276/container/openejb-loader/src/main/java/org/apache/openejb/loader/BasicURLClassPath.java >> > >> > >> > ---------------------------------------------------------------------- >> > >> > diff --git >> > >> > >> > >> >> > > >> > >> a/container/openejb-loader/src/main/java/org/apache/openejb/loader/BasicURLClassPath.java >> > >> > >> > >> >> > > >> > >> b/container/openejb-loader/src/main/java/org/apache/openejb/loader/BasicURLClassPath.java >> > >> > index e0d4837..b4c8af1 100644 >> > >> > --- >> > >> > >> > >> >> > > >> > >> a/container/openejb-loader/src/main/java/org/apache/openejb/loader/BasicURLClassPath.java >> > >> > +++ >> > >> > >> > >> >> > > >> > >> b/container/openejb-loader/src/main/java/org/apache/openejb/loader/BasicURLClassPath.java >> > >> > @@ -76,7 +76,7 @@ public abstract class BasicURLClassPath >> implements >> > >> > ClassPath { >> > >> > }); >> > >> > >> > >> > final URL[] jars = new URL[jarNames.length]; >> > >> > - final boolean isWindows = >> > > System.getProperty("os.name", >> > >> > "unknown").toLowerCase().startsWith("windows"); >> > >> > + final boolean isWindows = >> > > System.getProperty("os.name", >> > >> > "unknown").toLowerCase().startsWith("win"); >> > >> > >> > >> > for (int j = 0; j < jarNames.length; j++) { >> > >> > final String name = isWindows ? >> > jarNames[j].toLowerCase() >> > > : >> > >> > jarNames[j]; >> > >> > >> > >> > >> > >> > >> > >> >> > > >> > >> http://git-wip-us.apache.org/repos/asf/tomee/blob/bda2f276/container/openejb-loader/src/main/java/org/apache/openejb/loader/Files.java >> > >> > >> > ---------------------------------------------------------------------- >> > >> > diff --git >> > >> > >> > >> >> > > >> > >> a/container/openejb-loader/src/main/java/org/apache/openejb/loader/Files.java >> > >> > >> > >> >> > > >> > >> b/container/openejb-loader/src/main/java/org/apache/openejb/loader/Files.java >> > >> > index 1b0e6f8..04793f4 100644 >> > >> > --- >> > >> > >> > >> >> > > >> > >> a/container/openejb-loader/src/main/java/org/apache/openejb/loader/Files.java >> > >> > +++ >> > >> > >> > >> >> > > >> > >> b/container/openejb-loader/src/main/java/org/apache/openejb/loader/Files.java >> > >> > @@ -44,6 +44,7 @@ import static >> > >> > org.apache.openejb.loader.JarLocation.decode; >> > >> > */ >> > >> > public class Files { >> > >> > private static final Map<String, MessageDigest> DIGESTS = >> > > new >> > >> > HashMap<String, MessageDigest>(); >> > >> > + private static final boolean isWindows = >> > > System.getProperty(" >> > >> os.name", >> > >> > "unknown").toLowerCase().startsWith("win"); >> > >> > >> > >> > public static File path(final String... parts) { >> > >> > File dir = null; >> > >> > @@ -119,6 +120,8 @@ public class Files { >> > >> > if (!file.isDirectory()) { >> > >> > throw new FileRuntimeException("Not a directory: >> > > " + >> > >> > file.getAbsolutePath()); >> > >> > } >> > >> > + >> > >> > + System.gc(); >> > >> > return file; >> > >> > } >> > >> > >> > >> > @@ -167,6 +170,12 @@ public class Files { >> > >> > if (file.exists()) { >> > >> > return file; >> > >> > } >> > >> > + >> > >> > + if (isWindows) { >> > >> > + //Known Windows bug JDK-4715154 and as of JDK8 still >> not >> > >> > fixable due to OS >> > >> > + System.gc(); >> > >> > + } >> > >> > + >> > >> > if (!file.mkdirs()) { >> > >> > throw new FileRuntimeException("Cannot mkdir: " >> > > + >> > >> > file.getAbsolutePath()); >> > >> > } >> > >> > @@ -179,12 +188,18 @@ public class Files { >> > >> > >> > >> > public static File tmpdir() { >> > >> > try { >> > >> > - File file = null; >> > >> > + File file; >> > >> > try { >> > >> > file = File.createTempFile("temp", >> > > "dir"); >> > >> > } catch (final Throwable e) { >> > >> > //Use a local tmp directory >> > >> > final File tmp = new File("tmp"); >> > >> > + >> > >> > + if (isWindows) { >> > >> > + //Known Windows bug JDK-4715154 and as of >> JDK8 >> > > still >> > >> > not fixable due to OS >> > >> > + System.gc(); >> > >> > + } >> > >> > + >> > >> > if (!tmp.exists() && !tmp.mkdirs()) { >> > >> > throw new IOException("Failed to create >> > > local tmp >> > >> > directory: " + tmp.getAbsolutePath()); >> > >> > } >> > >> > @@ -192,6 +207,11 @@ public class Files { >> > >> > file = File.createTempFile("temp", >> > > "dir", tmp); >> > >> > } >> > >> > >> > >> > + if (isWindows) { >> > >> > + //Known Windows bug JDK-4715154 and as of JDK8 >> still >> > > not >> > >> > fixable due to OS >> > >> > + System.gc(); >> > >> > + } >> > >> > + >> > >> > if (!file.delete()) { >> > >> > throw new IOException("Failed to create temp >> > > dir. Delete >> > >> > failed"); >> > >> > } >> > >> > @@ -215,6 +235,11 @@ public class Files { >> > >> > >> > >> > if (!file.exists()) { >> > >> > >> > >> > + if (isWindows) { >> > >> > + //Known Windows bug JDK-4715154 and as of JDK8 >> still >> > > not >> > >> > fixable due to OS >> > >> > + System.gc(); >> > >> > + } >> > >> > + >> > >> > if (!file.mkdirs()) { >> > >> > throw new FileRuntimeException("Cannot mkdirs: >> > > " + >> > >> > file.getAbsolutePath()); >> > >> > } >> > >> > @@ -275,6 +300,7 @@ public class Files { >> > >> > } >> > >> > >> > >> > public static void delete(final File file) { >> > >> > + >> > >> > if (file.exists()) { >> > >> > if (file.isDirectory()) { >> > >> > final File[] files = file.listFiles(); >> > >> > @@ -285,6 +311,12 @@ public class Files { >> > >> > } >> > >> > } >> > >> > try { >> > >> > + >> > >> > + if (isWindows) { >> > >> > + //Known Windows bug JDK-4715154 and as of >> JDK8 >> > > still >> > >> > not fixable due to OS >> > >> > + System.gc(); >> > >> > + } >> > >> > + >> > >> > if (!file.delete()) { >> > >> > file.deleteOnExit(); >> > >> > } >> > >> > @@ -295,6 +327,7 @@ public class Files { >> > >> > } >> > >> > >> > >> > public static void remove(final File file) { >> > >> > + >> > >> > if (file == null) { >> > >> > return; >> > >> > } >> > >> > @@ -310,6 +343,12 @@ public class Files { >> > >> > } >> > >> > } >> > >> > } >> > >> > + >> > >> > + if (isWindows) { >> > >> > + //Known Windows bug JDK-4715154 and as of JDK8 still >> not >> > >> > fixable due to OS >> > >> > + System.gc(); >> > >> > + } >> > >> > + >> > >> > if (!file.delete()) { >> > >> > throw new IllegalStateException("Could not delete >> > > file: " + >> > >> > file.getAbsolutePath()); >> > >> > } >> > >> > >> > >> > >> > >> > >> > >> >> > > >> > >> http://git-wip-us.apache.org/repos/asf/tomee/blob/bda2f276/container/openejb-loader/src/test/java/org/apache/openejb/loader/provisining/MavenResolverTest.java >> > >> > >> > ---------------------------------------------------------------------- >> > >> > diff --git >> > >> > >> > >> >> > > >> > >> a/container/openejb-loader/src/test/java/org/apache/openejb/loader/provisining/MavenResolverTest.java >> > >> > >> > >> >> > > >> > >> b/container/openejb-loader/src/test/java/org/apache/openejb/loader/provisining/MavenResolverTest.java >> > >> > index 9988bcc..7d005c1 100644 >> > >> > --- >> > >> > >> > >> >> > > >> > >> a/container/openejb-loader/src/test/java/org/apache/openejb/loader/provisining/MavenResolverTest.java >> > >> > +++ >> > >> > >> > >> >> > > >> > >> b/container/openejb-loader/src/test/java/org/apache/openejb/loader/provisining/MavenResolverTest.java >> > >> > @@ -45,10 +45,10 @@ public class MavenResolverTest { >> > >> > >> > >> > @Test >> > >> > public void resolve() throws Exception { >> > >> > - File file = new File("target/test/foo1.jar"); >> > >> > + final File file = new File("target/test/foo.jar"); >> > >> > Files.remove(file); >> > >> > Files.mkdirs(file.getParentFile()); >> > >> > - FileOutputStream to = new FileOutputStream(file); >> > >> > + final FileOutputStream to = new FileOutputStream(file); >> > >> > >> > > IO.copy(resolver.resolve("mvn:junit:junit:4.12:jar"), to); >> > >> > IO.close(to); >> > >> > assertTrue(file.exists()); >> > >> > @@ -57,10 +57,10 @@ public class MavenResolverTest { >> > >> > >> > >> > @Test >> > >> > public void customRepo() throws Exception { >> > >> > - File file = new File("target/test/foo2.jar"); >> > >> > + final File file = new File("target/test/foo.jar"); >> > >> > Files.remove(file); >> > >> > Files.mkdirs(file.getParentFile()); >> > >> > - FileOutputStream to = new FileOutputStream(file); >> > >> > + final FileOutputStream to = new FileOutputStream(file); >> > >> > IO.copy(resolver.resolve("mvn: >> > >> > http://repo1.maven.org/maven2/!junit:junit:4.12:jar";), to); >> > >> > IO.close(to);d >> > >> > assertTrue(file.exists()); >> > >> > @@ -69,10 +69,10 @@ public class MavenResolverTest { >> > >> > >> > >> > @Test >> > >> > public void latest() throws Exception { >> > >> > - File file = new File("target/test/foo3.jar"); >> > >> > + final File file = new File("target/test/foo.jar"); >> > >> > Files.remove(file); >> > >> > Files.mkdirs(file.getParentFile()); >> > >> > - FileOutputStream to = new FileOutputStream(file); >> > >> > + final FileOutputStream to = new FileOutputStream(file); >> > >> > IO.copy(resolver.resolve("mvn: >> > >> > http://repo1.maven.org/maven2/!junit:junit:LATEST:jar";), to); >> > >> > IO.close(to); >> > >> > assertTrue(file.exists()); >> > >> > >> > >> > >> > >> >> > > >> > >> >
