Change all blanket catch statements to only catch Exception (#102)

* Change all blanket catch statements to only catch Exception

* Code review feedback

Author: BertKleewein

azure-iot-device/azure/iot/device/common/mqtt_provider.py

@@ -64,7 +64,7 @@ def on_connect(client, userdata, flags, rc):
             if self.on_mqtt_connected:
                 try:
                     self.on_mqtt_connected()
-                except:  # noqa: E722 do not use bare 'except'
+                except Exception:
                     logger.error("Unexpected error calling on_mqtt_connected")
                     logger.error(traceback.format_exc())
             else:
@@ -76,7 +76,7 @@ def on_disconnect(client, userdata, rc):
             if self.on_mqtt_disconnected:
                 try:
                     self.on_mqtt_disconnected()
-                except:  # noqa: E722 do not use bare 'except'
+                except Exception:
                     logger.error("Unexpected error calling on_mqtt_disconnected")
                     logger.error(traceback.format_exc())
             else:
@@ -103,7 +103,7 @@ def on_message(client, userdata, mqtt_message):
             if self.on_mqtt_message_received:
                 try:
                     self.on_mqtt_message_received(mqtt_message.topic, mqtt_message.payload)
-                except:  # noqa: E722 do not use bare 'except'
+                except Exception:
                     logger.error("Unexpected error calling on_mqtt_message_received")
                     logger.error(traceback.format_exc())
             else:
@@ -259,7 +259,7 @@ def establish_operation(self, mid, callback=None):
             if callback:
                 try:
                     callback()
-                except:  # noqa: E722 do not use bare 'except'
+                except Exception:
                     logger.error("Unexpected error calling callback for MID: {}".format(mid))
                     logger.error(traceback.format_exc())
             else:
@@ -301,7 +301,7 @@ def complete_operation(self, mid):
             if callback:
                 try:
                     callback()
-                except:  # noqa: E722 do not use bare 'except'
+                except Exception:
                     logger.error("Unexpected error calling callback for MID: {}".format(mid))
                     logger.error(traceback.format_exc())
             else:

azure-iot-device/azure/iot/device/common/pipeline/pipeline_stages_base.py

@@ -92,8 +92,7 @@ def run_op(self, op):
         logger.info("{}({}): running".format(self.name, op.name))
         try:
             self._run_op(op)
-        except:  # noqa: E722 do not use bare 'except'
-            _, e, _ = sys.exc_info()
+        except Exception as e:
             logger.error(msg="Error in {}._run_op() call".format(self), exc_info=e)
             op.error = e
             self.complete_op(op)
@@ -177,8 +176,7 @@ def on_finally_done(finally_op):
                             )
                         )
                         callback(finally_op)
-                    except:  # noqa: E722 do not use bare 'except'
-                        _, e, _ = sys.exc_info()
+                    except Exception as e:
                         logger.error(
                             msg="{}({}):run_ops_serial: Unhandled error in callback".format(
                                 self.name, finally_op.name
@@ -200,8 +198,7 @@ def on_finally_done(finally_op):
                         )
                     )
                     callback(last_op)
-                except:  # noqa: E722 do not use bare 'except'
-                    _, e, _ = sys.exc_info()
+                except Exception as e:
                     logger.error(
                         msg="{}({}):run_ops_serial: Unhandled error in callback".format(
                             self.name, last_op.name
@@ -257,8 +254,7 @@ def handle_pipeline_event(self, event):
         """
         try:
             self._handle_pipeline_event(event)
-        except:  # noqa: E722 do not use bare 'except'
-            _, e, _ = sys.exc_info()
+        except Exception as e:
             logger.error(
                 msg="Error in %s._handle_pipeline_event call".format(self.name), exc_info=e
             )
@@ -317,7 +313,7 @@ def complete_op(self, op):
         )
         try:
             op.callback(op)
-        except:  # noqa: E722 do not use bare 'except'
+        except Exception as e:
             _, e, _ = sys.exc_info()
             logger.error(
                 msg="Unhandled error calling back inside {}.complete_op() after {} complete".format(

azure-iot-device/azure/iot/device/common/pipeline/pipeline_stages_mqtt.py

@@ -83,9 +83,9 @@ def on_connected():
             self.provider.on_mqtt_connected = on_connected
             try:
                 self.provider.connect(self.sas_token)
-            except BaseException:
+            except Exception as e:
                 self.provider.on_mqtt_connected = self.on_connected
-                raise
+                raise e
 
         elif isinstance(op, pipeline_ops_base.Reconnect):
             logger.info("{}({}): reconnecting".format(self.name, op.name))
@@ -100,9 +100,9 @@ def on_connected():
             self.provider.on_mqtt_connected = on_connected
             try:
                 self.provider.reconnect(self.sas_token)
-            except BaseException:
+            except Exception as e:
                 self.provider.on_mqtt_connected = self.on_connected
-                raise
+                raise e
 
         elif isinstance(op, pipeline_ops_base.Disconnect):
             logger.info("{}({}): disconneting".format(self.name, op.name))
@@ -117,9 +117,9 @@ def on_disconnected():
             self.provider.on_mqtt_disconnected = on_disconnected
             try:
                 self.provider.disconnect()
-            except BaseException:
+            except Exception as e:
                 self.provider.on_mqtt_disconnected = self.on_disconnected
-                raise
+                raise e
 
         elif isinstance(op, pipeline_ops_mqtt.Publish):
             logger.info("{}({}): publishing on {}".format(self.name, op.name, op.topic))

azure-iot-device/azure/iot/device/iothub/pipeline/pipeline_stages_iothub.py

@@ -32,6 +32,11 @@ class UseSkAuthProvider(PipelineStage):
 
     def _run_op(self, op):
         if isinstance(op, pipeline_ops_iothub.SetAuthProvider):
+
+            def pipeline_ops_done(completed_op):
+                op.error = completed_op.error
+                op.callback(op)
+
             auth_provider = op.auth_provider
             self.run_ops_serial(
                 pipeline_ops_iothub.SetAuthProviderArgs(
@@ -42,7 +47,7 @@ def _run_op(self, op):
                     ca_cert=getattr(auth_provider, "ca_cert", None),
                 ),
                 pipeline_ops_base.SetSasToken(sas_token=auth_provider.get_current_sas_token()),
-                callback=op.callback,
+                callback=pipeline_ops_done,
             )
         else:
             self.continue_op(op)

azure-iot-device/azure/iot/device/provisioning/internal/polling_machine.py

@@ -419,7 +419,7 @@ def _on_disconnect_completed_error(self):
             self._register_callback = None
             try:
                 callback(None, self._registration_error)
-            except:  # noqa: E722 do not use bare 'except'
+            except Exception:
                 logger.error("Unexpected error calling callback supplied to register")
                 logger.error(traceback.format_exc())
 
@@ -439,7 +439,7 @@ def _on_disconnect_completed_register(self):
             self._register_callback = None
             try:
                 callback(self._registration_result, None)
-            except:  # noqa: E722 do not use bare 'except'
+            except Exception:
                 logger.error("Unexpected error calling callback supplied to register")
                 logger.error(traceback.format_exc())
 

azure-iot-device/azure/iot/device/provisioning/pipeline/pipeline_stages_provisioning.py

@@ -33,6 +33,11 @@ class UseSymmetricKeySecurityClient(PipelineStage):
 
     def _run_op(self, op):
         if isinstance(op, pipeline_ops_provisioning.SetSymmetricKeySecurityClient):
+
+            def pipeline_ops_done(completed_op):
+                op.error = completed_op.error
+                op.callback(op)
+
             security_client = op.security_client
             self.run_ops_serial(
                 pipeline_ops_provisioning.SetSymmetricKeySecurityClientArgs(
@@ -41,7 +46,7 @@ def _run_op(self, op):
                     id_scope=security_client.id_scope,
                 ),
                 pipeline_ops_base.SetSasToken(sas_token=security_client.get_current_sas_token()),
-                callback=op.callback,
+                callback=pipeline_ops_done,
             )
         else:
             self.continue_op(op)

azure-iot-device/tests/common/conftest.py

@@ -5,7 +5,7 @@
 # --------------------------------------------------------------------------
 
 import sys
-from .pipeline_test_fixtures import callback, fake_error, event
+from .pipeline_test_fixtures import callback, fake_exception, fake_base_exception, event
 
 collect_ignore = []
 

azure-iot-device/tests/common/pipeline/test_pipeline_stages_base.py

@@ -16,6 +16,7 @@
     make_mock_stage,
     assert_callback_failed,
     assert_callback_succeeded,
+    UnhandledException,
 )
 
 logging.basicConfig(level=logging.INFO)
@@ -130,14 +131,22 @@ def test_failed_operation(self, stage, callback, op):
         stage.run_ops_serial(op, callback=callback)
         assert_callback_failed(op=op, callback=callback)
 
-    @pytest.mark.it("protects the callback with a try/except block")
-    def test_exception_in_callback(self, stage, mocker, fake_error, op):
-        callback = mocker.Mock(side_effect=fake_error)
+    @pytest.mark.it(
+        "handles Exceptions raised in the callback and passes them to the unhandled error handler"
+    )
+    def test_callback_throws_exception(self, stage, mocker, fake_exception, op):
+        callback = mocker.Mock(side_effect=fake_exception)
         stage.run_ops_serial(op, callback=callback)
         assert callback.call_count == 1
         assert callback.call_args == mocker.call(op)
         assert stage.unhandled_error_handler.call_count == 1
-        assert stage.unhandled_error_handler.call_args == mocker.call(fake_error)
+        assert stage.unhandled_error_handler.call_args == mocker.call(fake_exception)
+
+    @pytest.mark.it("Allows any BaseExceptions raised in the callback to propagate")
+    def test_callback_throws_base_exception(self, stage, mocker, fake_base_exception, op):
+        callback = mocker.Mock(side_effect=fake_base_exception)
+        with pytest.raises(UnhandledException):
+            stage.run_ops_serial(op, callback=callback)
 
 
 @pytest.mark.describe("PipelineStage run_ops_serial function with one op and finally op")
@@ -196,14 +205,24 @@ def test_callback_with_error_if_op_fails_and_finally_op_succeeds(
         stage.run_ops_serial(op, finally_op=finally_op, callback=callback)
         assert_callback_failed(callback=callback, op=finally_op, error=op.error)
 
-    @pytest.mark.it("protects the callback with a try/except block")
-    def test_exception_in_callback(self, stage, op, finally_op, fake_error, mocker):
-        callback = mocker.Mock(side_effect=fake_error)
+    @pytest.mark.it(
+        "handles Exceptions raised in the callback and passes them to the unhandled error handler"
+    )
+    def test_callback_raises_exception(self, stage, op, finally_op, fake_exception, mocker):
+        callback = mocker.Mock(side_effect=fake_exception)
         stage.run_ops_serial(op, finally_op=finally_op, callback=callback)
         assert callback.call_count == 1
         assert callback.call_args == mocker.call(finally_op)
         assert stage.unhandled_error_handler.call_count == 1
-        assert stage.unhandled_error_handler.call_args == mocker.call(fake_error)
+        assert stage.unhandled_error_handler.call_args == mocker.call(fake_exception)
+
+    @pytest.mark.it("Allows any BaseExceptions raised in the callback to propagate")
+    def test_callback_raises_base_exception(
+        self, stage, op, finally_op, fake_base_exception, mocker
+    ):
+        callback = mocker.Mock(side_effect=fake_base_exception)
+        with pytest.raises(UnhandledException):
+            stage.run_ops_serial(op, finally_op=finally_op, callback=callback)
 
 
 @pytest.mark.describe("PipelineStage run_ops_serial function with three ops and without finally op")
@@ -231,14 +250,22 @@ def test_calls_callback_when_first_op_fails(self, stage, op, op2, op3, callback)
         stage.run_ops_serial(op, op2, op3, callback=callback)
         assert_callback_failed(callback=callback, op=op)
 
-    @pytest.mark.it("protects the callback with a try/except block")
-    def test_exception_in_callback(self, stage, op, op2, op3, fake_error, mocker):
-        callback = mocker.Mock(side_effect=fake_error)
+    @pytest.mark.it(
+        "handles Exceptions raised in the callback and passes them to the unhandled error handler"
+    )
+    def test_callback_raises_exception(self, stage, op, op2, op3, fake_exception, mocker):
+        callback = mocker.Mock(side_effect=fake_exception)
         stage.run_ops_serial(op, op2, op3, callback=callback)
         assert callback.call_count == 1
         assert callback.call_args == mocker.call(op3)
         assert stage.unhandled_error_handler.call_count == 1
-        assert stage.unhandled_error_handler.call_args == mocker.call(fake_error)
+        assert stage.unhandled_error_handler.call_args == mocker.call(fake_exception)
+
+    @pytest.mark.it("Allows any BaseExceptions raised in the callback to propagate")
+    def test_callback_raises_base_exception(self, stage, op, op2, op3, fake_base_exception, mocker):
+        callback = mocker.Mock(side_effect=fake_base_exception)
+        with pytest.raises(UnhandledException):
+            stage.run_ops_serial(op, op2, op3, callback=callback)
 
     @pytest.mark.it("runs the second op only after the first op succeeds")
     def test_runs_second_op_after_first_op_succceeds(self, mocker, stage, op, op2, op3, callback):
@@ -338,12 +365,24 @@ def test_calls_callbacK_with_error_when_first_op_and_finally_op_both_fail(
         stage.run_ops_serial(op, op2, op3, finally_op=finally_op, callback=callback)
         assert_callback_failed(callback=callback, op=finally_op, error=finally_op.error)
 
-    @pytest.mark.it("protects the callback with a try/except block")
-    def test_exception_in_callback(self, stage, op, op2, op3, finally_op, fake_error, mocker):
-        callback = mocker.Mock(side_effect=fake_error)
+    @pytest.mark.it(
+        "handles Exceptions raised in the callback and passes them to the unhandled error handler"
+    )
+    def test_callback_raises_exception(
+        self, stage, op, op2, op3, finally_op, fake_exception, mocker
+    ):
+        callback = mocker.Mock(side_effect=fake_exception)
         stage.run_ops_serial(op, op2, op3, callback=callback, finally_op=finally_op)
         assert stage.unhandled_error_handler.call_count == 1
-        assert stage.unhandled_error_handler.call_args == mocker.call(fake_error)
+        assert stage.unhandled_error_handler.call_args == mocker.call(fake_exception)
+
+    @pytest.mark.it("Allows any BaseExceptions raised in the callback to propagate")
+    def test_callback_raises_base_exception(
+        self, stage, op, op2, op3, finally_op, fake_base_exception, mocker
+    ):
+        callback = mocker.Mock(side_effect=fake_base_exception)
+        with pytest.raises(UnhandledException):
+            stage.run_ops_serial(op, op2, op3, callback=callback, finally_op=finally_op)
 
     @pytest.mark.it("runs the second op only after the first op succeeds")
     def test_runs_second_op(self, mocker, stage, op, op2, op3, finally_op, callback):
@@ -488,12 +527,22 @@ def test_calls_private_handle_pipeline_event(self, stage, event, mocker):
         assert stage._handle_pipeline_event.call_count == 1
         assert stage._handle_pipeline_event.call_args == mocker.call(event)
 
-    @pytest.mark.it("protects _handle_pipeline_event with a try/except block")
-    def test_exception_in_provate_handle_pipeline_event(self, stage, event, fake_error, mocker):
-        stage._handle_pipeline_event = mocker.Mock(side_effect=fake_error)
+    @pytest.mark.it(
+        "handles Exceptions raised in _handle_pipeline_Event and passes them to the unhandled error handler"
+    )
+    def test_handle_pipeline_events_raises_exception(self, stage, event, fake_exception, mocker):
+        stage._handle_pipeline_event = mocker.Mock(side_effect=fake_exception)
         stage.handle_pipeline_event(event)
         assert stage.unhandled_error_handler.call_count == 1
-        assert stage.unhandled_error_handler.call_args == mocker.call(fake_error)
+        assert stage.unhandled_error_handler.call_args == mocker.call(fake_exception)
+
+    @pytest.mark.it("Allows any BaseExceptions raised in _handle_pipeline_event to propagate")
+    def test_handle_pipeline_events_raises_base_exception(
+        self, stage, event, fake_base_exception, mocker
+    ):
+        stage._handle_pipeline_event = mocker.Mock(side_effect=fake_base_exception)
+        with pytest.raises(UnhandledException):
+            stage.handle_pipeline_event(event)
 
 
 @pytest.mark.describe("PipelineStage _handle_pipeline_event function")
@@ -516,12 +565,11 @@ def test_error_if_no_previous_stage(self, stage, event):
 @pytest.mark.describe("PipelineStage continue_op function")
 class TestPipelineStageContinueOp(object):
     @pytest.mark.it("completes the op without continuing if the op has an error")
-    def test_completes_op_with_error(self, mocker, stage, op, fake_error, callback):
-        op.error = fake_error
+    def test_completes_op_with_error(self, mocker, stage, op, fake_exception, callback):
+        op.error = fake_exception
         op.callback = callback
         stage.continue_op(op)
-        assert callback.call_count == 1
-        assert callback.call_args == mocker.call(op)
+        assert_callback_failed(op=op, error=fake_exception)
         assert stage.next.run_op.call_count == 0
 
     @pytest.mark.it("fails the op if there is no next stage")
@@ -548,20 +596,28 @@ def test_calls_callback_on_success(self, stage, op, callback):
         assert_callback_succeeded(op)
 
     @pytest.mark.it("calls the op callback on failure")
-    def test_calls_callback_on_error(self, stage, op, callback, fake_error):
-        op.error = fake_error
+    def test_calls_callback_on_error(self, stage, op, callback, fake_exception):
+        op.error = fake_exception
         op.callback = callback
         stage.complete_op(op)
-        assert_callback_failed(op=op, error=fake_error)
+        assert_callback_failed(op=op, error=fake_exception)
 
-    @pytest.mark.it("protects the op callback with a try/except handler")
-    def test_exception_in_callback(self, stage, op, fake_error, mocker):
-        op.callback = mocker.Mock(side_effect=fake_error)
+    @pytest.mark.it(
+        "handles Exceptions raised in operation callback and passes them to the unhandled error handler"
+    )
+    def test_op_callback_raises_exception(self, stage, op, fake_exception, mocker):
+        op.callback = mocker.Mock(side_effect=fake_exception)
         stage.complete_op(op)
         assert op.callback.call_count == 1
         assert op.callback.call_args == mocker.call(op)
         assert stage.unhandled_error_handler.call_count == 1
-        assert stage.unhandled_error_handler.call_args == mocker.call(fake_error)
+        assert stage.unhandled_error_handler.call_args == mocker.call(fake_exception)
+
+    @pytest.mark.it("Allows any BaseExceptions raised in operation callback to propagate")
+    def test_op_callback_raises_base_exception(self, stage, op, fake_base_exception, mocker):
+        op.callback = mocker.Mock(side_effect=fake_base_exception)
+        with pytest.raises(UnhandledException):
+            stage.complete_op(op)
 
 
 @pytest.mark.describe("PipelineStage continue_with_different_op function")