You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by gaohoward <gi...@git.apache.org> on 2018/01/22 14:56:44 UTC
[GitHub] activemq-artemis pull request #1800: ARTEMIS-1626 Disable thread leak check ...
GitHub user gaohoward opened a pull request:
https://github.com/apache/activemq-artemis/pull/1800
ARTEMIS-1626 Disable thread leak check for failing tests
The ThreadLeakCheckRule is used to check thread leaks
after each test is finished. However when a test fails, it is
not necessary to check leaking threads because the test
failure should be fixed anyway. And leaking threads in a
failed test may well be a result of the failure (once the test
is fixed the thread leak may be gone).
If a failed test also leaks threads, it takes a long time before
the thread leak check finishes (60 seconds checking time),
thus it takes a long time to finish, especially when tests are
run in batches with failures.
So to improve this, it should be reasonable to just enable
the thread leaking check for each test passes, and disable
the check when a test fails.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gaohoward/activemq-artemis fthread_rule
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1800.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 #1800
----
commit c8e25c72b9885bb08ec9dd5d8502c273b349d46a
Author: Howard Gao <ho...@...>
Date: 2018-01-22T14:39:23Z
ARTEMIS-1626 Disable thread leak check for failing tests
The ThreadLeakCheckRule is used to check thread leaks
after each test is finished. However when a test fails, it is
not necessary to check leaking threads because the test
failure should be fixed anyway. And leaking threads in a
failed test may well be a result of the failure (once the test
is fixed the thread leak may be gone).
If a failed test also leaks threads, it takes a long time before
the thread leak check finishes (60 seconds checking time),
thus it takes a long time to finish, especially when tests are
run in batches with failures.
So to improve this, it should be reasonable to just enable
the thread leaking check for each test passes, and disable
the check when a test fails.
----
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
I think the more interesting case is that a 'passing' test that leaking threads. If that's the case it's very hard to debug. But for a failing test, leaking threads is not that meaningful. In batch mode, you need to fix the first failing test, which are sure 'not' caused by leaking threads and should be a real failure. Any other failing tests can be ignored until the first is fixed.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@clebertsuconic If a failing test causes thread leaking and causes other tests to fail, the thread checking can't fix it right?
---
[GitHub] activemq-artemis pull request #1800: ARTEMIS-1626 Disable thread leak check ...
Posted by gaohoward <gi...@git.apache.org>.
GitHub user gaohoward reopened a pull request:
https://github.com/apache/activemq-artemis/pull/1800
ARTEMIS-1626 Disable thread leak check for failing tests
The ThreadLeakCheckRule is used to check thread leaks
after each test is finished. However when a test fails, it is
not necessary to check leaking threads because the test
failure should be fixed anyway. And leaking threads in a
failed test may well be a result of the failure (once the test
is fixed the thread leak may be gone).
If a failed test also leaks threads, it takes a long time before
the thread leak check finishes (60 seconds checking time),
thus it takes a long time to finish, especially when tests are
run in batches with failures.
So to improve this, it should be reasonable to just enable
the thread leaking check for each test passes, and disable
the check when a test fails.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gaohoward/activemq-artemis fthread_rule
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1800.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 #1800
----
commit c8e25c72b9885bb08ec9dd5d8502c273b349d46a
Author: Howard Gao <ho...@...>
Date: 2018-01-22T14:39:23Z
ARTEMIS-1626 Disable thread leak check for failing tests
The ThreadLeakCheckRule is used to check thread leaks
after each test is finished. However when a test fails, it is
not necessary to check leaking threads because the test
failure should be fixed anyway. And leaking threads in a
failed test may well be a result of the failure (once the test
is fixed the thread leak may be gone).
If a failed test also leaks threads, it takes a long time before
the thread leak check finishes (60 seconds checking time),
thus it takes a long time to finish, especially when tests are
run in batches with failures.
So to improve this, it should be reasonable to just enable
the thread leaking check for each test passes, and disable
the check when a test fails.
----
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
failed tests are also supposed to cleanup their resources...
example
try {
}
finally {
close()
}
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@mtaylor the opposite.. we could have it enabled on the dev and test profiles.. and have it off by default. just like checkstyle is off on dev profile.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
All we asked was logging instead.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@gaohoward test1 failed.. leaked.. than test2 failed because there's a thread leak... if you don't report the leak from 1.. you won't know that test2 could fail later..
so.. add a profile to disable the thread leak check would be enough.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
There's a leak in your changes... Every test will create a new instance of ThreadLeakCheckRule.. and it will be added into UnitTestWatcher.. at the end of the testsuite this will be fairly slow
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@clebertsuconic I don't think the PR make the debugging of thread leaking harder.
For example suppose we have 3 tests, test1, test2 and test3 running in this order.
Let's say test1 passes and test2 fails. You can be sure test2 is not caused by leaking threads from test1, because test1 passes and the thread checking rule is enabled on it.
So you just need to fix test2 (in which thread leaking factor is ruled out).
This is the same as before (without this PR), but the tests will be finished 60 second earlier.
This is good when a PR is submitted and Jenkins starts a job. If the PR causes any failure,
jenkins will finish faster and the PR owner will quickly get the report and starting investigate.
Does that make sense?
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@gaohoward I'm ok on not reporting a failure.. but it should always log a leakage.. even for failed tests..
We could instead of failing on leakage.. log.warn instead after a test failed... but simply ignore a leakage is not good.
---
[GitHub] activemq-artemis pull request #1800: ARTEMIS-1626 Disable thread leak check ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1800
---
[GitHub] activemq-artemis pull request #1800: ARTEMIS-1626 Disable thread leak check ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1800#discussion_r163259914
--- Diff: artemis-commons/src/test/java/org/apache/activemq/artemis/utils/ThreadLeakCheckRule.java ---
@@ -257,11 +262,13 @@ private boolean isExpectedThread(Thread thread) {
@Override
public void testSucceeded(Description description) {
this.enabled = true;
--- End diff --
Hmmmm.. you removed disable(). why?
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@gaohoward as I said.. I have been the one to fix these freaking issues when the testsuite is acting up... so... I would be okay with your changes if you at least logged the leakages without hiding the previous reason why the test failed.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@gaohoward I undestand your points you've showed on my questions, but as @clebertsuconic has pointed
> the ideal world is far from reality.. there are tests that will be failing forever on the testsuite and people will ignore them
I agree with that and I think we should try to keep more isolated than possible (if possible) tests (failing or not) just for this reason: just logging the leaks would be enough for me.
While dealing with very long running tests who will read the test reports need to rely that 2 different runs are comparable somehow and hiding this information won't help doing it.
I understand the need to make the process more agile and to aim "0 tests failures" and I think that just logging would be great.
I hope to have explained better what I'm worried about
---
[GitHub] activemq-artemis pull request #1800: ARTEMIS-1626 Disable thread leak check ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1800#discussion_r163257308
--- Diff: artemis-commons/src/test/java/org/apache/activemq/artemis/utils/ThreadLeakCheckRule.java ---
@@ -257,11 +262,13 @@ private boolean isExpectedThread(Thread thread) {
@Override
public void testSucceeded(Description description) {
this.enabled = true;
--- End diff --
remove this.enabled from here.
There's a method to disable the leak.. you would be breaking it here.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
I think logging would eventually turn on the thread checking, that's the original behavior, no need to change anything, right?
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
We are supposed to write tests to cleanup resources.. even when it fails.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@clebertsuconic
regarding the thread leak logging, I don't think I got it right. Do you mean if test failed, we give the thread logging, what about test that pases? (just keep in mind if test failed we sure will check and log without this PR)
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@clebertsuconic re: "test1 failed.. leaked.. than test2 failed because there's a thread leak... if you don't report the leak from 1.. you won't know that test2 could fail later.."
In that case test1 should be looked at first. after test1 is corrected the thread check rule kicks in to make sure it doesn't leak, therefore won't pollute test2.
The harder case is test1 passed..leaked.. then test2 failed becasue of the leak. Even worse case is that test1 passed leaked and tests runs on until test(n) failed due to the test1's leak. This PR doesn't allow that happen because it checks every passing test for thread leaks.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
I have redone this with a much simpler change... please review it.. I will make a direct commit on master.
---
[GitHub] activemq-artemis pull request #1800: ARTEMIS-1626 Disable thread leak check ...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1800
---
[GitHub] activemq-artemis pull request #1800: ARTEMIS-1626 Disable thread leak check ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1800#discussion_r163257179
--- Diff: artemis-commons/src/test/java/org/apache/activemq/artemis/utils/ThreadLeakCheckRule.java ---
@@ -257,11 +262,13 @@ private boolean isExpectedThread(Thread thread) {
@Override
public void testSucceeded(Description description) {
this.enabled = true;
+ this.testFailed = false;
}
@Override
public void testFailed(Throwable e, Description description) {
- this.enabled = false;
+ this.testFailed = true;
--- End diff --
remove this.enabled from here... just this.testFailed
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@franz1981 If the test fails, whether cleanly or not, we get reported and should fix it. The leakage is not important here. After you fix it and it begin to pass, the thread check rule will perform check on it, making sure it doesn't leak threads. Let's say there are 2 situations:
1) first test passes but secretly leaking threads and caused second test fail.
2) first test failed and also leaking thread and caused second test fail.
IMO the case 1) would be much harder to debug. So in case 1) the first test should be checked to make sure it doesn't pollute others. In case 2) however, you know the first test failed and you can be sure it fails on its own, there for a real failure. So no matter how many more failures follow, you will have to fix the first failure first. Once it passes, the thread check rule kicks in to make sure the leak won't happen.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@clebertsuconic can you take a look at the additional commit ?
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@gaohoward
I understand, but often a failed test is expected to fail cleanly (without having leaking threads): having that information ignored could affect anyway the other tests and probably it is hiding leaks that is better to know (eg oracle JDBC has leaking driver clean threads that is better to have it reported on failed tests too: thay won't stop never even if the thread is killed manually).
Have I misunderstood the change?
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
How about we just have a system variable to switch off the thread check rule for debug purposes. Default being on. -DapplyThreadCheckRule=true
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
-1 I don't agree with this... a thread leaking on a failing test will make another test to fail.
This is about raising awareness of leaked threads.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@clebertsuconic @gaohoward
Some time ago has happened that I've written a test that was failing and I adming that having the thread leak check has helped me to find that a test wasn't failing gently ie letting the system be in the most resilient state.
I think that would be perfect to disable it just for rare cases where really the priority is just to pass the test
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@gaohoward I have been there before... finding a test that's failing because of a previous leak it's not fun... I don't want to back into that...
If a thread is leaking a thread.. failing or not.. I would rather like to see the thread leakage reported.. so I would know where it's coming from...
I don't want to regress on that.. I had spent a week trying to find a correlation between tests before.. hence I'm being a bit strong opinionated here.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@clebertsuconic @mtaylor @franz1981 We shouldn't disable the thread check for all tests. What this PR do it selectively disable the check for failing tests. If a test passes the check is certainly on, to make sure it silently leaking and affects other tests.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
Right now it fails the test. If you changed it to log only if the test failed. You got what you wanted. And what we wanted. It’s a good compromise.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
Why close it instead of logging it ?
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
a failing test leaking is a double failure... it failed and then leaked.. you can fix that situation with finally blocks...
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
ok, I think it's better to take cautions as people worries about it.
Thanks you guys for the opinions !
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
reopen for further discussion.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
so, I don't mean to be disrespectul.. your changes are nice technically.. but I don't want to lose visibility of leaking threads.. even if it's for a failed test.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
also.. you cannot have the junit-test depending on the test-jar.. I'm removing the dependency and keeping the rule as it was...
junit-test is to be used by external users... I'm keeping it as is.
I'm changing the ThreadRuleCheck to extend TestWatcher.. it's a much simpler change... no other classes changed.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@gaohoward the idea world is far from reality.. there are tests that will be failing forever on the testsuite and people will ignore them.. a result will be a mess that people won't know where the leaks are coming from again.
The ThreadLeak Rule has prevented leaked threads to fail non related tests..
and the threadleak rule is not only logging, but it's also trying to interrupt these leaked threads...
Lets not regress on that please!
We can do a faster check (not wait for a whole minute on failted tests.. and we would only logg. instead of fail).. it's a simple change.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
Please don’t close those. Just change it to log.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
> @clebertsuconic I'm not convinced that it is that important to log a leakage on a failed test. If a test
> fails it should be fixed. With this PR you won't miss a real leakage.
What.. you don't even want to log a leakage? that's information we need to determine if subsequent tests may fail.
A test shouldn't leak threads.. even if it failed.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@gaohoward I'm handling it.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
@clebertsuconic I'm not convinced that it is that important to log a leakage on a failed test. If a test fails it should be fixed. With this PR you won't miss a real leakage.
---
[GitHub] activemq-artemis issue #1800: ARTEMIS-1626 Disable thread leak check for fai...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1800
We already have a dev profile. Perhaps it could be disabled there.
---