Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2067533002:
Check for stray threads from tests and fail if found (Closed)
|
Created:
4 years, 6 months ago by Sergiy Byelozyorov Modified:
4 years, 6 months ago Reviewers:
tandrii(chromium) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/infra/testing/testing_support.git@master Target Ref:
refs/heads/master Project:
infra/testing/testing_support Visibility:
Public. |
DescriptionCheck for stray threads from tests and fail if found
R=tandrii@chromium.org
BUG=619481, 619610
Committed: https://chromium.googlesource.com/infra/testing/testing_support/+/ab9c80db669731ff1acdbe8f2f2a36433def4ba1
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressed comments #
Total comments: 2
Patch Set 3 : Addressed comments #
Total comments: 2
Patch Set 4 : Fix #Patch Set 5 : Addressed comments #
Total comments: 2
Patch Set 6 : Addressed comments #
Messages
Total messages: 16 (6 generated)
Description was changed from ========== Check for stray threads from tests and fail if so COMMIT=false (infra may still have some tests that need to be fixed) R=tandrii@chromium.org BUG=619481 ========== to ========== Check for stray threads from tests and fail if found COMMIT=false (infra may still have some tests that need to be fixed) R=tandrii@chromium.org BUG=619481 ==========
Description was changed from ========== Check for stray threads from tests and fail if found COMMIT=false (infra may still have some tests that need to be fixed) R=tandrii@chromium.org BUG=619481 ========== to ========== Check for stray threads from tests and fail if found R=tandrii@chromium.org BUG=619481 ==========
PTAL and CQ. This can land now, but should only be deployed (via deps.pyl) after all infra tests are fixed, see http://crbug.com/619610.
Description was changed from ========== Check for stray threads from tests and fail if found R=tandrii@chromium.org BUG=619481 ========== to ========== Check for stray threads from tests and fail if found R=tandrii@chromium.org BUG=619481, 619610 ==========
https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.py File testing_support/auto_stub.py (right): https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.p... testing_support/auto_stub.py:67: class TestCase(thread_watcher.TestCase, AutoStubMixIn): i'd not touch this one. Provide your testcase, yes, but don't make it default. Instead, switch to it to default on a case by case basis. https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.p... testing_support/auto_stub.py:71: super(TestCase, self).tearDown() this is bad. It should be explicit as it used to be before. https://codereview.chromium.org/2067533002/diff/1/testing_support/tests/threa... File testing_support/tests/thread_watcher_test.py (right): https://codereview.chromium.org/2067533002/diff/1/testing_support/tests/threa... testing_support/tests/thread_watcher_test.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. nit 2016 https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... File testing_support/thread_watcher.py (right): https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... testing_support/thread_watcher.py:1: # Copyright (c) 2011 The Chromium Authors. All rights reserved. nit 2016 https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... testing_support/thread_watcher.py:12: self._pre_test_threads = [t.ident for t in threading.enumerate()] strictly speaking, these numbers can be re-used. See https://docs.python.org/3/library/threading.html#threading.Thread.ident So, the code is below is correct when the failure is reported, but it may actually loose some cases. https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... testing_support/thread_watcher.py:19: len(new_threads), ', '.join(t.name for t in new_threads))) t.name is usually not given, at least in infra land. Thus, this line is not helpful. It's better to report a stack trace for each thread.
https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.py File testing_support/auto_stub.py (right): https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.p... testing_support/auto_stub.py:67: class TestCase(thread_watcher.TestCase, AutoStubMixIn): On 2016/06/13 17:03:27, tandrii(chromium) wrote: > i'd not touch this one. Provide your testcase, yes, but don't make it default. > Instead, switch to it to default on a case by case basis. Developers are used to inherit from auto_stub.TestCase or trial_dir.TestCase. Given this, all new tests that are added will not have thread_watcher built-in. I'd rather have all new tests written to leak no threads. https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.p... testing_support/auto_stub.py:71: super(TestCase, self).tearDown() On 2016/06/13 17:03:27, tandrii(chromium) wrote: > this is bad. It should be explicit as it used to be before. We do it differently everywhere else. Why should it be explicit? In fact, if it's explicit we then have to update this call site each time we choose to change the base class. https://codereview.chromium.org/2067533002/diff/1/testing_support/tests/threa... File testing_support/tests/thread_watcher_test.py (right): https://codereview.chromium.org/2067533002/diff/1/testing_support/tests/threa... testing_support/tests/thread_watcher_test.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/06/13 17:03:28, tandrii(chromium) wrote: > nit 2016 Done. https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... File testing_support/thread_watcher.py (right): https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... testing_support/thread_watcher.py:1: # Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2016/06/13 17:03:28, tandrii(chromium) wrote: > nit 2016 Done. https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... testing_support/thread_watcher.py:12: self._pre_test_threads = [t.ident for t in threading.enumerate()] On 2016/06/13 17:03:28, tandrii(chromium) wrote: > strictly speaking, these numbers can be re-used. > > See https://docs.python.org/3/library/threading.html#threading.Thread.ident > > So, the code is below is correct when the failure is reported, but it may > actually loose some cases. Agree, but I am not sure how otherwise we can check it. All threads that existed before test should belong to the test launcher and shouldn't stop due to the actions of the test. I can perhaps add thread name to the identification, but I doubt it will make things much better. https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... testing_support/thread_watcher.py:19: len(new_threads), ', '.join(t.name for t in new_threads))) On 2016/06/13 17:03:28, tandrii(chromium) wrote: > t.name is usually not given, at least in infra land. Thus, this line is not > helpful. It's better to report a stack trace for each thread. t.name defaults to Thread-NN, where NN-some unique number. It's just as good as thread.ident. Regarding stacktraces - I'm not quite sure how to do them in Python. In most places where I looked, I see suggestions to use sys._current_frames(), but I'm somewhat worried about using private methods from the standard library. IMHO, from the context of the CL introducing this error it's likely clear what threads were started and not shut down. Also I've named all threads in commit_queue (but not elsewhere in infra/infra_internal). However, if you think using sys._current_frames is okay, then I'll use it. WDYT?
i'm OK with CL as is, but I think stack traces will ease debugging this by almost 100%. https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.py File testing_support/auto_stub.py (right): https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.p... testing_support/auto_stub.py:67: class TestCase(thread_watcher.TestCase, AutoStubMixIn): On 2016/06/14 06:12:14, Sergiy Byelozyorov wrote: > On 2016/06/13 17:03:27, tandrii(chromium) wrote: > > i'd not touch this one. Provide your testcase, yes, but don't make it default. > > Instead, switch to it to default on a case by case basis. > > Developers are used to inherit from auto_stub.TestCase or trial_dir.TestCase. > Given this, all new tests that are added will not have thread_watcher built-in. > I'd rather have all new tests written to leak no threads. it's a trade off for how easy it'd be to roll this into infra. But I think long-term, I'm 100% with you. Thus, if you keep this as it is, then you should make sure all tests are fixed (which is good in itself, but may take time). https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.p... testing_support/auto_stub.py:71: super(TestCase, self).tearDown() On 2016/06/14 06:12:14, Sergiy Byelozyorov wrote: > On 2016/06/13 17:03:27, tandrii(chromium) wrote: > > this is bad. It should be explicit as it used to be before. > > We do it differently everywhere else. Why should it be explicit? In fact, if > it's explicit we then have to update this call site each time we choose to > change the base class. Oh, yes, I suppose it's Ok. I prefer explicit because I don't understand the order of resolution in super() when class inherits from several classes. https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... File testing_support/thread_watcher.py (right): https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... testing_support/thread_watcher.py:19: len(new_threads), ', '.join(t.name for t in new_threads))) On 2016/06/14 06:12:14, Sergiy Byelozyorov wrote: > On 2016/06/13 17:03:28, tandrii(chromium) wrote: > > t.name is usually not given, at least in infra land. Thus, this line is not > > helpful. It's better to report a stack trace for each thread. > > t.name defaults to Thread-NN, where NN-some unique number. It's just as good as > thread.ident. Yes, and as result both suck for debugging, unless you carefully name every thread created (like you did for CQ :) ). That's why stack traces are much more useful. > Regarding stacktraces - I'm not quite sure how to do them in > Python. In most places where I looked, I see suggestions to use > sys._current_frames(), but I'm somewhat worried about using private methods from > the standard library. IMHO, from the context of the CL introducing this error > it's likely clear what threads were started and not shut down. Well, depends on the CL. Even you did naming of the CQ threads, so I guess it wasn't clear :) > Also I've named > all threads in commit_queue (but not elsewhere in infra/infra_internal). > However, if you think using sys._current_frames is okay, then I'll use it. WDYT? It's OK since both Py2 and Py3 has it, and we aren't going to move away from CPython. I personally use this way: http://stackoverflow.com/a/24334576 https://codereview.chromium.org/2067533002/diff/20001/testing_support/thread_... File testing_support/thread_watcher.py (right): https://codereview.chromium.org/2067533002/diff/20001/testing_support/thread_... testing_support/thread_watcher.py:24: """Base unittest class that cleans off a trial directory in tearDown().""" update doc
https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.py File testing_support/auto_stub.py (right): https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.p... testing_support/auto_stub.py:67: class TestCase(thread_watcher.TestCase, AutoStubMixIn): On 2016/06/14 10:21:43, tandrii(chromium) wrote: > On 2016/06/14 06:12:14, Sergiy Byelozyorov wrote: > > On 2016/06/13 17:03:27, tandrii(chromium) wrote: > > > i'd not touch this one. Provide your testcase, yes, but don't make it > default. > > > Instead, switch to it to default on a case by case basis. > > > > Developers are used to inherit from auto_stub.TestCase or trial_dir.TestCase. > > Given this, all new tests that are added will not have thread_watcher > built-in. > > I'd rather have all new tests written to leak no threads. > > it's a trade off for how easy it'd be to roll this into infra. But I think > long-term, I'm 100% with you. Thus, if you keep this as it is, then you should > make sure all tests are fixed (which is good in itself, but may take time). Acknowledged. https://codereview.chromium.org/2067533002/diff/1/testing_support/auto_stub.p... testing_support/auto_stub.py:71: super(TestCase, self).tearDown() On 2016/06/14 10:21:43, tandrii(chromium) wrote: > On 2016/06/14 06:12:14, Sergiy Byelozyorov wrote: > > On 2016/06/13 17:03:27, tandrii(chromium) wrote: > > > this is bad. It should be explicit as it used to be before. > > > > We do it differently everywhere else. Why should it be explicit? In fact, if > > it's explicit we then have to update this call site each time we choose to > > change the base class. > > Oh, yes, I suppose it's Ok. I prefer explicit because I don't understand the > order of resolution in super() when class inherits from several classes. Acknowledged. https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... File testing_support/thread_watcher.py (right): https://codereview.chromium.org/2067533002/diff/1/testing_support/thread_watc... testing_support/thread_watcher.py:19: len(new_threads), ', '.join(t.name for t in new_threads))) On 2016/06/14 10:21:43, tandrii(chromium) wrote: > On 2016/06/14 06:12:14, Sergiy Byelozyorov wrote: > > On 2016/06/13 17:03:28, tandrii(chromium) wrote: > > > t.name is usually not given, at least in infra land. Thus, this line is not > > > helpful. It's better to report a stack trace for each thread. > > > > t.name defaults to Thread-NN, where NN-some unique number. It's just as good > as > > thread.ident. > Yes, and as result both suck for debugging, unless you carefully name every > thread created (like you did for CQ :) ). That's why stack traces are much more > useful. > > > Regarding stacktraces - I'm not quite sure how to do them in > > Python. In most places where I looked, I see suggestions to use > > sys._current_frames(), but I'm somewhat worried about using private methods > from > > the standard library. IMHO, from the context of the CL introducing this error > > it's likely clear what threads were started and not shut down. > Well, depends on the CL. Even you did naming of the CQ threads, so I guess it > wasn't clear :) > > > Also I've named > > all threads in commit_queue (but not elsewhere in infra/infra_internal). > > However, if you think using sys._current_frames is okay, then I'll use it. > WDYT? > It's OK since both Py2 and Py3 has it, and we aren't going to move away from > CPython. > I personally use this way: http://stackoverflow.com/a/24334576 Done. https://codereview.chromium.org/2067533002/diff/20001/testing_support/thread_... File testing_support/thread_watcher.py (right): https://codereview.chromium.org/2067533002/diff/20001/testing_support/thread_... testing_support/thread_watcher.py:24: """Base unittest class that cleans off a trial directory in tearDown().""" On 2016/06/14 10:21:43, tandrii(chromium) wrote: > update doc Done.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2067533002/diff/60001/testing_support/thread_... File testing_support/thread_watcher.py (right): https://codereview.chromium.org/2067533002/diff/60001/testing_support/thread_... testing_support/thread_watcher.py:25: # This can happen due to threads starting or stopping concurrently. true, but then deal with it fully: try: detail = traceback.format_stack(sys._current_frames[t.ident]) except: if t.ident in current_frames: raise # This can happen due to threads starting or stopping concurrently.
https://codereview.chromium.org/2067533002/diff/60001/testing_support/thread_... File testing_support/thread_watcher.py (right): https://codereview.chromium.org/2067533002/diff/60001/testing_support/thread_... testing_support/thread_watcher.py:25: # This can happen due to threads starting or stopping concurrently. On 2016/06/14 20:06:24, tandrii(chromium) wrote: > true, but then deal with it fully: > > try: > detail = traceback.format_stack(sys._current_frames[t.ident]) > except: > if t.ident in current_frames: > raise > # This can happen due to threads starting or stopping concurrently. Done.
Patchset #5 (id:100001) has been deleted
LGTM % nit https://codereview.chromium.org/2067533002/diff/120001/testing_support/thread... File testing_support/thread_watcher.py (right): https://codereview.chromium.org/2067533002/diff/120001/testing_support/thread... testing_support/thread_watcher.py:30: details.append(' Thread already stopped.\n') s/already stopped/stopped while acquiring stacktrace.
https://codereview.chromium.org/2067533002/diff/120001/testing_support/thread... File testing_support/thread_watcher.py (right): https://codereview.chromium.org/2067533002/diff/120001/testing_support/thread... testing_support/thread_watcher.py:30: details.append(' Thread already stopped.\n') On 2016/06/14 20:26:57, tandrii(chromium) wrote: > s/already stopped/stopped while acquiring stacktrace. Done.
Description was changed from ========== Check for stray threads from tests and fail if found R=tandrii@chromium.org BUG=619481, 619610 ========== to ========== Check for stray threads from tests and fail if found R=tandrii@chromium.org BUG=619481, 619610 Committed: https://chromium.googlesource.com/infra/testing/testing_support/+/ab9c80db669... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) manually as ab9c80db669731ff1acdbe8f2f2a36433def4ba1 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
