PR#845: propagate exception correctly - koji

koji

#845 propagate exception correctly

Merged 5 years ago by mikem. Opened 6 years ago by tkopecek.

tkopecek/koji issue844 into master

remove unused variable

Tomas Kopecek • 5 years ago

3b14806

propagate exception correctly

Tomas Kopecek • 5 years ago

633bfe1

koji/tasks.py

file modified

+14 -16

		`@@ -366,7 +366,10 @@`
		`"""`

		`if canfail is None:`
		`- canfail = []`
		`+ checked = set()`
		`+ else:`
		`+ # canfail task are marked as checked`
		`+ checked = set(canfail)`
		`if isinstance(subtasks, int):`
		`# allow single integer w/o enclosing list`
		`subtasks = [subtasks]`
		`@@ -381,23 +384,18 @@`
		`elif len(finished) > 0:`
		`if all:`
		`if failany:`
		`- failed = False`
		`- for task in finished:`
		`- if task in canfail:`
		`- # no point in checking`
		`- continue`
		`+ # we care only about tasks which are not correctly`
		`+ # finished and in same time not in canfail list`
		`+ for task in set(finished) - checked:`
		`try:`
		`self.session.getTaskResult(task)`
		`- except (koji.GenericError, xmlrpclib.Fault) as task_error:`
		`- self.logger.info("task %s failed or was canceled" % task)`
		`- failed = True`
		`- break`
		`- if failed:`
		`- self.logger.info("at least one task failed or was canceled, cancelling unfinished tasks")`
		`- self.session.cancelTaskChildren(self.id)`
		`- # reraise the original error now, rather than waiting for`
		`- # an error in taskWaitResults()`
		`- raise task_error`
		`+ checked.add(task)`
		`+ except (koji.GenericError, xmlrpclib.Fault):`
		`+ self.logger.info("task %s failed or was canceled, cancelling unfinished tasks" % task)`
		`+ self.session.cancelTaskChildren(self.id)`
		`+ # reraise the original error now, rather than waiting for`
		`+ # an error in taskWaitResults()`
		`+ raise`
		`else:`
		`# at least one done`
		`break`

tests/test_lib_py2only/test_tasks.py

file modified

+3 -3

		`@@ -215,7 +215,7 @@`
		`obj.session.host.taskWaitResults.return_value = taskWaitResults`
		`self.assertEquals(obj.wait([1551234, 1591234]), dict(taskWaitResults))`
		`obj.session.host.taskSetWait.assert_called_once_with(12345678, [1551234, 1591234])`
		`- obj.session.host.taskWaitResults.assert_called_once_with(12345678, [1551234, 1591234], canfail=[])`
		`+ obj.session.host.taskWaitResults.assert_called_once_with(12345678, [1551234, 1591234], canfail=None)`

		`def test_BaseTaskHandler_wait_some_not_done(self):`
		`""" Tests that the wait function returns the one finished subtask results of`
		`@@ -240,7 +240,7 @@`
		`obj.session.host.taskWaitResults.return_value = taskWaitResults`
		`self.assertEquals(obj.wait([1551234, 1591234]), dict(taskWaitResults))`
		`obj.session.host.taskSetWait.assert_called_once_with(12345678, [1551234, 1591234])`
		`- obj.session.host.taskWaitResults.assert_called_once_with(12345678, [1551234], canfail=[])`
		`+ obj.session.host.taskWaitResults.assert_called_once_with(12345678, [1551234], canfail=None)`

		`@patch('signal.pause', return_value=None)`
		`def test_BaseTaskHandler_wait_some_not_done_all_set(self, mock_signal_pause):`
		`@@ -280,7 +280,7 @@`
		`obj.session.host.taskSetWait.assert_called_once_with(12345678, [1551234, 1591234])`
		`obj.session.host.taskWait.assert_has_calls([call(12345678), call(12345678)])`
		`mock_signal_pause.assert_called_once_with()`
		`- obj.session.host.taskWaitResults.assert_called_once_with(12345678, [1551234, 1591234], canfail=[])`
		`+ obj.session.host.taskWaitResults.assert_called_once_with(12345678, [1551234, 1591234], canfail=None)`

		`def test_BaseTaskHandler_wait_some_not_done_all_set_failany_set_failed_task(self):`
		`""" Tests that the wait function raises an exception when one of the subtask fails when the failany flag is set`

tkopecek commented 6 years ago

Fixes: https://pagure.io/koji/issue/844

julian8628 commented 6 years ago

:thumbsup:

mikem commented 6 years ago

Ah, I had forgotten that python3 added this. My brain still clings to the global/locals (+closure) scope model in py2.

https://stackoverflow.com/questions/45292479/name-binding-in-except-clause-deleted-after-the-clause

It's not technically implemented as a scope though. Granted, from a practical perspective, the difference is irrelevant.

This PEP intends to resolve this issue by adding a cleanup semantic to except clauses in Python 3 whereby the target name is deleted at the end of the except suite.

There are similar behavior changes with list comprehensions and generator expressions. Those are apperently implemented with function scopes.

mikem commented 6 years ago

Kind of a style nit, but having task_error shift from boolean to exception bugs me...

https://github.com/mikem23/koji-playground/commits/pagure/pr/845

mikem commented 6 years ago

Also, I'm trying to remember why we deferred raising the exception until after the loop.

This code has been refactored a few times and I think it used to matter, but I don't see how it does not. We get an error, we save the exception object, we break, and then we re-raise if we got the error. Looks like the flow would be the same if we just moved that bit into the exception handler, except we'd get a cleaner exception because we could just use a bare raise.

Or maybe I'm missing something in my haste...

rebased onto 54d720072ca345afd31f590838353a163606d5e4

6 years ago

tkopecek commented 6 years ago

I was going through history and that behaviour is there from beginning of git, so not sure what is there. I'm also not able to reinvent the reason, so I've tried to remove it. I've also added small optimization to not check for same result twice - it shouldn't hurt.

rebased onto 716092eb944dd4b71cd7d3b4a591d8722cc5e250

6 years ago

rebased onto 633bfe1

5 years ago

mikem commented 5 years ago

koji/tasks.py:393:76: F841 local variable 'ex' is assigned to but never used

mikem commented 5 years ago

Looks good otherwise

tkopecek commented 5 years ago

removed

1 new commit added