Nir Soffer has uploaded a new change for review.
Change subject: lib: Revert and refine error handling in tmap() ......................................................................
lib: Revert and refine error handling in tmap()
In commit 2b7155b696 (lib: Simplify and generalize concurrent.tmap()), we simplified error handling by returning a named tuple with function results. This turned out less useful then the original error handling.
This patch returns the previous error handling:
- Functions passed to tmap() should not raise - if they raise, this is considered a bug in the function. - The last error is raised by tmap() instead of returning the result. This make it easier to fail loudly for unexpected errors. - The original exception is re-raised now with the original traceback. - Error handling is documented properly now
Previously you had to make sure function raises to signal failures:
def func(): try: code that should not fail... code that may fail... code that should not fail... except ExpectedError: log.error(...) raise except Exception: log.exception(...) raise
results = concurrent.tmap(func, values)
if not all(r.succeeded for r in results): ...
Returning the result as is lets us have nicer code:
def func(): code that should not fail... try: code that may fail... except ExpectedError: log.error(...) return False code that should not fail... return True
succeeded = concurrent.tmap(func, values)
if not all(succeeded): ...
We can ignore unexpected errors, since tmap() will log them and fail loudly. We can also minimize try except block for expected errors.
Change-Id: I0154b28ff7822c63e77181bbbf444c712bd0c31e Signed-off-by: Nir Soffer nsoffer@redhat.com --- M lib/vdsm/concurrent.py M tests/concurrentTests.py 2 files changed, 45 insertions(+), 19 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/11/39211/1
diff --git a/lib/vdsm/concurrent.py b/lib/vdsm/concurrent.py index 64e072d..5498052 100644 --- a/lib/vdsm/concurrent.py +++ b/lib/vdsm/concurrent.py @@ -18,22 +18,42 @@ # Refer to the README and COPYING files for full details of the license #
+import logging import threading -from collections import namedtuple - - -Result = namedtuple("Result", ["succeeded", "value"]) +import sys
def tmap(func, iterable): + """ + Run func with arguments from iterable in multiple threads, a returning the + output in order of arguments. + + func should not raise exceptions - we consider this a bug in func, and will + fail the call and re-raise the exception in the caller thread. + + Expected exceptions should be handled in func. If the caller likes to + handle the error later, func should return it: + + def func(value): + try: + return something(value) + except ExpectedError as e: + return e + + Unexpected exceptions should not be handled, as they are logged in the + worker threads and re-raised in the caller thread. If multiple excpetions + raised, only the last one will be re-raised in the caller thread. + """ args = list(iterable) results = [None] * len(args) + error = [None]
def worker(i, f, arg): try: - results[i] = Result(True, f(arg)) - except Exception as e: - results[i] = Result(False, e) + results[i] = f(arg) + except Exception: + error[0] = sys.exc_info() + logging.exception("Unhandled exception in tmap worker thread")
threads = [] for i, arg in enumerate(args): @@ -45,4 +65,8 @@ for t in threads: t.join()
+ if error[0] is not None: + t, v, tb = error[0] + raise t, v, tb + return results diff --git a/tests/concurrentTests.py b/tests/concurrentTests.py index 307e397..5c0646b 100644 --- a/tests/concurrentTests.py +++ b/tests/concurrentTests.py @@ -26,13 +26,16 @@ from vdsm import concurrent
+class Error(Exception): + pass + + class TMapTests(VdsmTestCase):
def test_results(self): values = tuple(range(10)) results = concurrent.tmap(lambda x: x, values) - expected = [concurrent.Result(True, x) for x in values] - self.assertEqual(results, expected) + self.assertEqual(results, list(values))
def test_results_order(self): def func(x): @@ -40,8 +43,7 @@ return x values = tuple(random.random() * 0.1 for x in range(10)) results = concurrent.tmap(func, values) - expected = [concurrent.Result(True, x) for x in values] - self.assertEqual(results, expected) + self.assertEqual(results, list(values))
def test_concurrency(self): start = time.time() @@ -49,12 +51,12 @@ elapsed = time.time() - start self.assertTrue(0.1 < elapsed < 0.2)
- def test_error(self): - error = RuntimeError("No result for you!") - + def test_raise_last_error(self): def func(x): - raise error - - results = concurrent.tmap(func, range(10)) - expected = [concurrent.Result(False, error)] * 10 - self.assertEqual(results, expected) + raise Error(x) + try: + concurrent.tmap(func, (1, 2, 3)) + except Error as e: + self.assertEqual(e.args, (3,)) + else: + self.fail("Exception was not raised")
automation@ovirt.org has posted comments on this change.
Change subject: lib: Revert and refine error handling in tmap() ......................................................................
Patch Set 1:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: lib: Revert and refine error handling in tmap() ......................................................................
Patch Set 1:
Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17143/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: lib: Revert and refine error handling in tmap() ......................................................................
Patch Set 1:
Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17317/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: lib: Revert and refine error handling in tmap() ......................................................................
Patch Set 1:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17143/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17317/ : FAILURE
Yaniv Bronhaim has posted comments on this change.
Change subject: lib: Revert and refine error handling in tmap() ......................................................................
Patch Set 1:
(3 comments)
https://gerrit.ovirt.org/#/c/39211/1//COMMIT_MSG Commit Message:
Line 52: Line 53: succeeded = concurrent.tmap(func, values) Line 54: Line 55: if not all(succeeded): Line 56: ... We knew that... I'm not sure I understand why you changed your mind. checking each result is less efficient but you keep the error for later use and I thought your usage could require that Line 57: Line 58: We can ignore unexpected errors, since tmap() will log them and fail Line 59: loudly. We can also minimize try except block for expected errors. Line 60:
https://gerrit.ovirt.org/#/c/39211/1/lib/vdsm/concurrent.py File lib/vdsm/concurrent.py:
Line 66: t.join() Line 67: Line 68: if error[0] is not None: Line 69: t, v, tb = error[0] Line 70: raise t, v, tb just raise error[0]. and if you raise, the caller doesn't need to check - if not all(succeeded), just cover with try block Line 71:
https://gerrit.ovirt.org/#/c/39211/1/tests/concurrentTests.py File tests/concurrentTests.py:
Line 56: raise Error(x) Line 57: try: Line 58: concurrent.tmap(func, (1, 2, 3)) Line 59: except Error as e: Line 60: self.assertEqual(e.args, (3,)) can't you just check assertRaises ? simpler .. Line 61: else:
Nir Soffer has posted comments on this change.
Change subject: lib: Revert and refine error handling in tmap() ......................................................................
Patch Set 1:
(3 comments)
https://gerrit.ovirt.org/#/c/39211/1//COMMIT_MSG Commit Message:
Line 52: Line 53: succeeded = concurrent.tmap(func, values) Line 54: Line 55: if not all(succeeded): Line 56: ...
We knew that... I'm not sure I understand why you changed your mind. checki
I did not like the code I had to write with the previous api. You can see that the next patches are nicer now because of this change. Line 57: Line 58: We can ignore unexpected errors, since tmap() will log them and fail Line 59: loudly. We can also minimize try except block for expected errors. Line 60:
https://gerrit.ovirt.org/#/c/39211/1/lib/vdsm/concurrent.py File lib/vdsm/concurrent.py:
Line 66: t.join() Line 67: Line 68: if error[0] is not None: Line 69: t, v, tb = error[0] Line 70: raise t, v, tb
just raise error[0]. and if you raise, the caller doesn't need to check - i
No, raising error[0] will loose the original tracback. This code will show the original traceback on the caller thread, so you have more context when debugging this.
For example, if you run this in the shell, you will get the original traceback form the tmap worker thread in the main thread.
And the caller *does* need to check if not all(succeeded), because what I'm doing in this change is handling differently expected errors (e.g, scsi scan failed) and unexpected errors (e.g, name error because of bug in the code).
tmap() will raise only if func raises (e.g. bug in the code), but will not raise if func had expected failure (there is some problem with the environment).
All this is explained in the commit message and in the function docstring. Line 71:
https://gerrit.ovirt.org/#/c/39211/1/tests/concurrentTests.py File tests/concurrentTests.py:
Line 56: raise Error(x) Line 57: try: Line 58: concurrent.tmap(func, (1, 2, 3)) Line 59: except Error as e: Line 60: self.assertEqual(e.args, (3,))
can't you just check assertRaises ? simpler ..
No, I want to check that the last error was raised, so I need the exception object. This is the simplest way to get it. Line 61: else:
Yaniv Bronhaim has posted comments on this change.
Change subject: lib: Revert and refine error handling in tmap() ......................................................................
Patch Set 1: Code-Review+1
although I liked the cleaner suggestion which confused me as I wasn't aware of the buildin map function that works as the old tmap code worked. federico - please use that code or revert last change nsoffer merged
Jenkins CI RO has abandoned this change.
Change subject: lib: Revert and refine error handling in tmap() ......................................................................
Abandoned
Abandoned due to no activity - please restore if still relevant
gerrit-hooks has posted comments on this change.
Change subject: lib: Revert and refine error handling in tmap() ......................................................................
Patch Set 1:
* Update tracker: IGNORE, no Bug-Url found
Nir Soffer has restored this change.
Change subject: lib: Revert and refine error handling in tmap() ......................................................................
Restored
vdsm-patches@lists.fedorahosted.org