fix the thread leak in timeout class (https://github.com/cucumber/cucumb... #640

Merged
merged 2 commits into from Dec 3, 2013

Conversation

Projects
None yet
5 participants
@volna80
Contributor

volna80 commented Dec 2, 2013

fix the thread leak in timeout class (cucumber#639)

  1. all tests are passed

  2. tested with my own business project which has a lot of scenarios, used yourkit to profile and confirmed that no thread leak now.

@@ -28,6 +27,7 @@ public void run() {
} finally {
done.set(true);
timer.cancel(true);
+ executorService.shutdownNow();

This comment has been minimized.

@ffbit

ffbit Dec 2, 2013

Contributor

This line of code only makes the real thing.

@ffbit

ffbit Dec 2, 2013

Contributor

This line of code only makes the real thing.

This comment has been minimized.

@volna80

volna80 Dec 2, 2013

Contributor

sure,

PS. haven't worked with github pull request. How can I do it? cancel pull request or raise new one?

@volna80

volna80 Dec 2, 2013

Contributor

sure,

PS. haven't worked with github pull request. How can I do it? cancel pull request or raise new one?

@ffbit

View changes

core/src/main/java/cucumber/runtime/Timeout.java
-import java.util.concurrent.ScheduledFuture;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.TimeoutException;
+import java.util.concurrent.*;

This comment has been minimized.

@ffbit

ffbit Dec 2, 2013

Contributor

Could you please consider reverting of these changes - wild card import isn't a project's common practice.

@ffbit

ffbit Dec 2, 2013

Contributor

Could you please consider reverting of these changes - wild card import isn't a project's common practice.

@ffbit

This comment has been minimized.

Show comment
Hide comment
@ffbit

ffbit Dec 2, 2013

Contributor

There at least 2 ways (in my humble opinion):

  • ammend an existent pulled commit in PR
  • make another commit in the same branch (you've already done this)

Also spend five minutes on reading the contribution guide.

Thanks for your contribution.

Contributor

ffbit commented Dec 2, 2013

There at least 2 ways (in my humble opinion):

  • ammend an existent pulled commit in PR
  • make another commit in the same branch (you've already done this)

Also spend five minutes on reading the contribution guide.

Thanks for your contribution.

@volna80

This comment has been minimized.

Show comment
Hide comment
@volna80

volna80 Dec 3, 2013

Contributor

Hi,

Are you expecting anything else for this PR?

I see that the build has finished with Error state, but couldn't understand what the error. I've seen that the build has passed according to maven output. Could somebody look at this? Is it possible to restart a build?

Contributor

volna80 commented Dec 3, 2013

Hi,

Are you expecting anything else for this PR?

I see that the build has finished with Error state, but couldn't understand what the error. I've seen that the build has passed according to maven output. Could somebody look at this? Is it possible to restart a build?

@brasmusson

This comment has been minimized.

Show comment
Hide comment
@brasmusson

brasmusson Dec 3, 2013

Contributor

@volna80 For us with write access to the cucumber-jvm repository it is possible to restart a build on Travis. I have restarted the job in the error state.

An advanced tip for pull requests on github:
If you edit the pull request body and include fixes #639 (or one of the other "closing" keywords), then issue #639 will automatically be closed when this pull request is merged, see https://help.github.com/articles/closing-issues-via-commit-messages#closing-issues-with-pull-requests

Contributor

brasmusson commented Dec 3, 2013

@volna80 For us with write access to the cucumber-jvm repository it is possible to restart a build on Travis. I have restarted the job in the error state.

An advanced tip for pull requests on github:
If you edit the pull request body and include fixes #639 (or one of the other "closing" keywords), then issue #639 will automatically be closed when this pull request is merged, see https://help.github.com/articles/closing-issues-via-commit-messages#closing-issues-with-pull-requests

@aslakhellesoy

This comment has been minimized.

Show comment
Hide comment
@aslakhellesoy

aslakhellesoy Dec 3, 2013

Contributor

Are you expecting anything else for this PR?

Yes - a unit test. You can use Thread.getAllStackTraces() to ensure that the number of threads remains constant after say, 100 invocations of timeout(). Also verify that no exceptions are thrown. @os97673 mentioned a risk of InterruptedException being thrown unless done.set(true) is called inside the try block.

Contributor

aslakhellesoy commented Dec 3, 2013

Are you expecting anything else for this PR?

Yes - a unit test. You can use Thread.getAllStackTraces() to ensure that the number of threads remains constant after say, 100 invocations of timeout(). Also verify that no exceptions are thrown. @os97673 mentioned a risk of InterruptedException being thrown unless done.set(true) is called inside the try block.

@aslakhellesoy

This comment has been minimized.

Show comment
Hide comment
@aslakhellesoy

aslakhellesoy Dec 3, 2013

Contributor

Sorry - I spoke too soon! I see there is a unit test now :-) I'll leave some comments in that test instead.

Contributor

aslakhellesoy commented Dec 3, 2013

Sorry - I spoke too soon! I see there is a unit test now :-) I'll leave some comments in that test instead.

+ final long finishNumberOfThreads = Thread.getAllStackTraces().size();
+
+ assertTrue("The number of threads have grown significantly. start: " + startNumberOfThreads + ", end:" + finishNumberOfThreads,
+ Math.abs(finishNumberOfThreads - startNumberOfThreads) < 5);

This comment has been minimized.

@aslakhellesoy

aslakhellesoy Dec 3, 2013

Contributor

This is a fragile test that will eventually fail on a particularly rainy day. It's what we call a flickering test.

I'm guessing you picked the number 5 arbitrarily because you saw the number of threads increase. Why has the number of threads increased by 5? Maybe because Timeout creates a new executor every time timeout() is called. Try making the executor a private static field so there is only ever one, then see if you can get the thread difference down to 1.

@aslakhellesoy

aslakhellesoy Dec 3, 2013

Contributor

This is a fragile test that will eventually fail on a particularly rainy day. It's what we call a flickering test.

I'm guessing you picked the number 5 arbitrarily because you saw the number of threads increase. Why has the number of threads increased by 5? Maybe because Timeout creates a new executor every time timeout() is called. Try making the executor a private static field so there is only ever one, then see if you can get the thread difference down to 1.

This comment has been minimized.

@brasmusson

brasmusson Dec 3, 2013

Contributor

To me an assertEquals(startNumberOfThreads, finishNumberOfThreads) seems to work, when the test case is given a bit extra time than the initial 10ms (see comment above).

@brasmusson

brasmusson Dec 3, 2013

Contributor

To me an assertEquals(startNumberOfThreads, finishNumberOfThreads) seems to work, when the test case is given a bit extra time than the initial 10ms (see comment above).

This comment has been minimized.

@os97673

os97673 Dec 3, 2013

Member

imho to make the code unit-testable you have to inject the executor (or something else) otherwise the test will be either unstable or slow.

@os97673

os97673 Dec 3, 2013

Member

imho to make the code unit-testable you have to inject the executor (or something else) otherwise the test will be either unstable or slow.

This comment has been minimized.

@volna80

volna80 Dec 3, 2013

Contributor

Frankly speaking,

I don't think that unit tests are design to catch such issues (like any resource leaks). Than we talk about real multi threading, obviously there is no guarantee from jvm that it will stop all threads in a given timeout. We don't know how exactly will behave Thread.getAllStackTraces() in future java versions and etc.

You want to have a unit test, I"ve written one. I believe it will be a stable. If you ask me again, I prefer just to delete this test and fix the bug.

The goal of this test that just to check that it doesn't create a huge amount of new threads.

@volna80

volna80 Dec 3, 2013

Contributor

Frankly speaking,

I don't think that unit tests are design to catch such issues (like any resource leaks). Than we talk about real multi threading, obviously there is no guarantee from jvm that it will stop all threads in a given timeout. We don't know how exactly will behave Thread.getAllStackTraces() in future java versions and etc.

You want to have a unit test, I"ve written one. I believe it will be a stable. If you ask me again, I prefer just to delete this test and fix the bug.

The goal of this test that just to check that it doesn't create a huge amount of new threads.

This comment has been minimized.

@aslakhellesoy

aslakhellesoy Dec 3, 2013

Contributor

Good points @volna80. Everything is merged and on master now. If this starts failing at some point I agree we can delete the test. For now, let's just leave it there - it seems ok.

@aslakhellesoy

aslakhellesoy Dec 3, 2013

Contributor

Good points @volna80. Everything is merged and on master now. If this starts failing at some point I agree we can delete the test. For now, let's just leave it there - it seems ok.

+ }
+ Thread.sleep(10);
+
+ final long finishNumberOfThreads = Thread.getAllStackTraces().size();

This comment has been minimized.

@brasmusson

brasmusson Dec 3, 2013

Contributor

It is a bit optimistic to think that all timeout threads have been deleted directly after 10ms (at least on Windows where I experimented to figure out if the < 5 was necessary). A safer construct is something like:

        long finishNumberOfThreads = Thread.getAllStackTraces().size();
        for (int i = 0; i < 100; i++) {
            if (finishNumberOfThreads == startNumberOfThreads) {
                break;
            }
            Thread.sleep(5);
            finishNumberOfThreads = Thread.getAllStackTraces().size();
        }

It allows for up to 500ms before the number of threads are back to the starting level, but does not sleep longer then on average 2,5ms longer than necessary. So far I have never seen more than a couple of loops actually been needed in practice.

@brasmusson

brasmusson Dec 3, 2013

Contributor

It is a bit optimistic to think that all timeout threads have been deleted directly after 10ms (at least on Windows where I experimented to figure out if the < 5 was necessary). A safer construct is something like:

        long finishNumberOfThreads = Thread.getAllStackTraces().size();
        for (int i = 0; i < 100; i++) {
            if (finishNumberOfThreads == startNumberOfThreads) {
                break;
            }
            Thread.sleep(5);
            finishNumberOfThreads = Thread.getAllStackTraces().size();
        }

It allows for up to 500ms before the number of threads are back to the starting level, but does not sleep longer then on average 2,5ms longer than necessary. So far I have never seen more than a couple of loops actually been needed in practice.

This comment has been minimized.

@volna80

volna80 Dec 3, 2013

Contributor

I won't be so optimistics and predict that any other thread won't spawn one more new thread. We don't control the env. I think we should just focus on the main thing. It doesn't spawn enormous number of new threads (thread leak detect).

@volna80

volna80 Dec 3, 2013

Contributor

I won't be so optimistics and predict that any other thread won't spawn one more new thread. We don't control the env. I think we should just focus on the main thing. It doesn't spawn enormous number of new threads (thread leak detect).

@aslakhellesoy aslakhellesoy merged commit 5ca341d into cucumber:master Dec 3, 2013

1 check passed

default The Travis CI build passed
Details
@aslakhellesoy

This comment has been minimized.

Show comment
Hide comment
@aslakhellesoy

aslakhellesoy Dec 3, 2013

Contributor

I merged this in with some changes. If anyone from the team wants to improve it, please feel free. It runs pretty fast though, and I think it's pretty reliable now, thanks to @brasmusson's suggestion.

Contributor

aslakhellesoy commented Dec 3, 2013

I merged this in with some changes. If anyone from the team wants to improve it, please feel free. It runs pretty fast though, and I think it's pretty reliable now, thanks to @brasmusson's suggestion.

@brasmusson

This comment has been minimized.

Show comment
Hide comment
@brasmusson

brasmusson Dec 11, 2013

Contributor

@aslakhellesoy The TimeoutTest has been failing from time to time, for me more often when executed from Eclipse than when executed from maven. Now it also has failed on Travis (https://travis-ci.org/cucumber/cucumber-jvm/builds/15295806). One source of failure I tracked down was that some (one) extra pool threads exist when the initial thread count is samples, so the thread count is actually decreasing (slightly) during the test. I changed the test to allow for decreasing number of threads in 77a93d1

Contributor

brasmusson commented Dec 11, 2013

@aslakhellesoy The TimeoutTest has been failing from time to time, for me more often when executed from Eclipse than when executed from maven. Now it also has failed on Travis (https://travis-ci.org/cucumber/cucumber-jvm/builds/15295806). One source of failure I tracked down was that some (one) extra pool threads exist when the initial thread count is samples, so the thread count is actually decreasing (slightly) during the test. I changed the test to allow for decreasing number of threads in 77a93d1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment