[GitHub] maven-surefire pull request #144: Resource leaks.

[stephenc]

Github user stephenc commented on a diff in the pull request:

https://github.com/apache/maven-surefire/pull/144#discussion_r103110391
  
--- Diff: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/ConsoleOutputFileReporter.java
 ---
@@ -69,8 +69,8 @@ public void close()
 {
 try
 {
-fileOutputStream.flush();
-fileOutputStream.close();
+// fileOutputStream.flush(); Will not call close on 
exception!
+fileOutputStream.close(); // Will implicitly flush.
--- End diff --

Then the correct way to do that is wrap the flush in another layer of try 
catch (and leave a todo to use addSuppressed once baseline JVM for the code is 
Java 7)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

[GitHub] maven-surefire pull request #144: Resource leaks.

[ChristianSchulte]

Github user ChristianSchulte commented on a diff in the pull request:

https://github.com/apache/maven-surefire/pull/144#discussion_r103110250
  
--- Diff: 
surefire-booter/src/main/java/org/apache/maven/surefire/booter/ForkedBooter.java
 ---
@@ -236,13 +236,17 @@ private static void exit( int returnCode, Shutdown 
shutdownType, CommandReader r
 switch ( shutdownType )
 {
 case KILL:
+System.out.close();
--- End diff --

`System.in` and `System.out` are both 
[`PrintStream`](http://hg.openjdk.java.net/jdk7u/jdk7u/jdk/file/8108ffe38ccb/src/share/classes/java/io/PrintStream.java)s.
 That class already is error prone like mad (autoFlush yes or no, checkError 
instead of throwing exceptions, etc.). It internally creates a `BufferedWriter` 
used for writing characters. I haven't found anything in the JDK flushing those 
buffers so it seems this is up to the application. I am quite sure that closing 
those streams manually is correct. That's what you would do in a C application 
as well.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

[GitHub] maven-surefire pull request #144: Resource leaks.

[ChristianSchulte]

Github user ChristianSchulte commented on a diff in the pull request:

https://github.com/apache/maven-surefire/pull/144#discussion_r103110034
  
--- Diff: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/ConsoleOutputFileReporter.java
 ---
@@ -69,8 +69,8 @@ public void close()
 {
 try
 {
-fileOutputStream.flush();
-fileOutputStream.close();
+// fileOutputStream.flush(); Will not call close on 
exception!
+fileOutputStream.close(); // Will implicitly flush.
--- End diff --

I just wanted to be sure that `close` is always called. See the comment. 
When `flush` throws an exception, `close` will never be called and leak 
resources.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

[GitHub] maven-surefire pull request #144: Resource leaks.

[ChristianSchulte]

Github user ChristianSchulte commented on a diff in the pull request:

https://github.com/apache/maven-surefire/pull/144#discussion_r103109929
  
--- Diff: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/StatelessXmlReporter.java
 ---
@@ -240,6 +240,10 @@ public void testSetCompleted( WrappedReportEntry 
testSetReportEntry, TestSetStat
 }
 ppw.endElement(); // TestSuite
 }
+catch ( final IOException e )
+{
+throw new ReporterException( "Failure generating XML report.", 
e );
--- End diff --

I started a surefire build and then left because it runs a long time and I 
did not want to wait. There were a couple of ITs no longer failing. I'll write 
an email when getting home. Results looked promising on OpenBSD. Last commit is 
just temporary for testing. I closed various maven-shared-utils issues today 
which seem to fix things for surefire as well. I'll send an email when I am 
back. I hope the console will show a successful build. 


[CommandLineUtils](http://svn.apache.org/viewvc?view=revision=1784432)
[StreamFeeder](http://svn.apache.org/viewvc?view=revision=1784431)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

[GitHub] maven-surefire pull request #144: Resource leaks.

[Tibor17]

Github user Tibor17 commented on a diff in the pull request:

https://github.com/apache/maven-surefire/pull/144#discussion_r103107177
  
--- Diff: 
surefire-booter/src/main/java/org/apache/maven/surefire/booter/ForkedBooter.java
 ---
@@ -236,13 +236,17 @@ private static void exit( int returnCode, Shutdown 
shutdownType, CommandReader r
 switch ( shutdownType )
 {
 case KILL:
+System.out.close();
--- End diff --

The logs show me that `Process.exec()` is blocked with maybe read() method 
in another Thread, and maybe a kind of deadlock which means using error stream 
might be a good try.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

[GitHub] maven-surefire pull request #144: Resource leaks.

[Tibor17]

Github user Tibor17 commented on a diff in the pull request:

https://github.com/apache/maven-surefire/pull/144#discussion_r103107040
  
--- Diff: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/ConsoleOutputFileReporter.java
 ---
@@ -69,8 +69,8 @@ public void close()
 {
 try
 {
-fileOutputStream.flush();
-fileOutputStream.close();
+// fileOutputStream.flush(); Will not call close on 
exception!
+fileOutputStream.close(); // Will implicitly flush.
--- End diff --

Writer needs close() without flush() but FileOutputStream does not have 
implicit flush while closing. Javadoc does not mention it. Implementation in 
Oracle JDK is maybe different and probably flushes on close but that's this JVM 
vendor.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

[GitHub] maven-surefire pull request #144: Resource leaks.

[Tibor17]

Github user Tibor17 commented on a diff in the pull request:

https://github.com/apache/maven-surefire/pull/144#discussion_r103106957
  
--- Diff: 
surefire-booter/src/main/java/org/apache/maven/surefire/booter/ForkedBooter.java
 ---
@@ -236,13 +236,17 @@ private static void exit( int returnCode, Shutdown 
shutdownType, CommandReader r
 switch ( shutdownType )
 {
 case KILL:
+System.out.close();
--- End diff --

@ChristianSchulte 
This is executed right before JVM exit.
But our problem is in the beginning of life time of JVM.
This means the `InputStream.read()` and (`Process.waitFor()` or 
`Process.exec()`) are mutual exclusive on FreeBSD according to logs.
It seems std/err will be tested instead of std/out.
Additionally ACK before shutdown needs to be added as StephenC mentioned.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

[GitHub] maven-surefire pull request #144: Resource leaks.

[Tibor17]

Github user Tibor17 commented on a diff in the pull request:

https://github.com/apache/maven-surefire/pull/144#discussion_r103106853
  
--- Diff: 
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/report/StatelessXmlReporter.java
 ---
@@ -240,6 +240,10 @@ public void testSetCompleted( WrappedReportEntry 
testSetReportEntry, TestSetStat
 }
 ppw.endElement(); // TestSuite
 }
+catch ( final IOException e )
+{
+throw new ReporterException( "Failure generating XML report.", 
e );
--- End diff --

@ChristianSchulte 
Pls do not commit. I have stash with explanation.
Later. First of all is to test freebasd.
I have fixed testng system properties in a branch but have not commit it to 
the branch. We will test it. But later in the evening.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org

[GitHub] maven-surefire pull request #144: Resource leaks.

[ChristianSchulte]

GitHub user ChristianSchulte opened a pull request:

https://github.com/apache/maven-surefire/pull/144

Resource leaks.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ChristianSchulte/maven-surefire master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/maven-surefire/pull/144.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #144


commit 3530cc137cbeb12002212a516bc2d41a769b3274
Author: Christian Schulte 
Date:   2017-02-18T22:37:27Z

o Resource leak in 'RunEntryStatisticsMap'.

commit 9f1ace227eef2d8e9c2bb9dbf66b848f0ffeec9e
Author: Christian Schulte 
Date:   2017-02-18T22:40:38Z

o Resource leak in 'ConsoleOutputFileReporter'.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
For additional commands, e-mail: dev-h...@maven.apache.org