You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@maven.apache.org by ChristianSchulte <gi...@git.apache.org> on 2017/02/18 22:48:30 UTC
[GitHub] maven-surefire pull request #144: Resource leaks.
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 <cs...@schulte.it>
Date: 2017-02-18T22:37:27Z
o Resource leak in 'RunEntryStatisticsMap'.
commit 9f1ace227eef2d8e9c2bb9dbf66b848f0ffeec9e
Author: Christian Schulte <cs...@schulte.it>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #144: Resource leaks.
Posted by Tibor17 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #144: Resource leaks.
Posted by ChristianSchulte <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #144: Resource leaks.
Posted by Tibor17 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire issue #144: Resource leaks.
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on the issue:
https://github.com/apache/maven-surefire/pull/144
But most of these changes are not related to communication between JVMs.
I spoke with Michael-O and we know the root cause. The problem is that the
JVM does not receive data via std/in in forked JVM. Therefore the forked
JVM is still waiting for class to test. It is not related to
System.out.close(); in ForkedBooter.
On Sun, Feb 26, 2017 at 6:14 PM, Stephen Connolly <no...@github.com>
wrote:
> *@stephenc* commented on this pull request.
> ------------------------------
>
> In maven-surefire-common/src/main/java/org/apache/maven/
> plugin/surefire/report/ConsoleOutputFileReporter.java
> <https://github.com/apache/maven-surefire/pull/144#discussion_r103110391>:
>
> > @@ -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.
>
> 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)
>
> \u2014
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/maven-surefire/pull/144#discussion_r103110391>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AA_yR4iQ5XcirBYLEhH3UZ9ue48U_3mnks5rgbMAgaJpZM4MFSQu>
> .
>
--
Cheers
Tibor
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #144: Resource leaks.
Posted by ChristianSchulte <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #144: Resource leaks.
Posted by ChristianSchulte <gi...@git.apache.org>.
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&revision=1784432)
[StreamFeeder](http://svn.apache.org/viewvc?view=revision&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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #144: Resource leaks.
Posted by stephenc <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #144: Resource leaks.
Posted by Tibor17 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #144: Resource leaks.
Posted by Tibor17 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org