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" <agumbre...@tomitribe.com> 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 <strub...@yahoo.de> 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 <
> > rmannibu...@gmail.com> 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"
> > > <jlmonte...@tomitribe.com> 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, <andygumbre...@apache.org> 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: andygumbre...@apache.org <andy...@gmx.de>
> > >> > Authored: Thu Mar 12 10:07:25 2015 +0100
> > >> > Committer: andygumbre...@apache.org <andy...@gmx.de>
> > >> > 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());
> > >> >
> > >> >
> > >>
> > >
> >
>
