Repository: scrapy/scrapy
Issue category: Bug Report

return in finally can swallow exception
### Description

There are two places in `scrapy/contracts/__init__.py` where a `finally:` body contains a `return` statement, which would swallow any in-flight exception. 

This means that if a `BaseException` (such as `KeyboardInterrupt`) is raised from the body, or any exception is raised from one of the `except:` clause, it will not propagate on as expected. 

The pylint warning about this was suppressed in [this commit](https://github.com/scrapy/scrapy/commit/991121fa91aee4d428ae09e75427d4e91970a41b) but it doesn't seem like there was justification for that.

These are the two locations:
https://github.com/scrapy/scrapy/blame/b4bad97eae6bcce790a626d244c63589f4134408/scrapy/contracts/__init__.py#L56

https://github.com/scrapy/scrapy/blame/b4bad97eae6bcce790a626d244c63589f4134408/scrapy/contracts/__init__.py#L86

See also https://docs.python.org/3/tutorial/errors.html#defining-clean-up-actions.


Functions to edit:
scrapy/contracts/__init__.py:Contract.add_pre_hook
scrapy/contracts/__init__.py:Contract.add_post_hook

Patch:
diff --git a/scrapy/contracts/__init__.py b/scrapy/contracts/__init__.py
index 9071395e3d9..3b4f932a014 100644
--- a/scrapy/contracts/__init__.py
+++ b/scrapy/contracts/__init__.py
@@ -38,9 +38,7 @@ def add_pre_hook(self, request: Request, results: TestResult) -> Request:
             assert cb is not None
 
             @wraps(cb)
-            def wrapper(  # pylint: disable=inconsistent-return-statements
-                response: Response, **cb_kwargs: Any
-            ) -> list[Any]:
+            def wrapper(response: Response, **cb_kwargs: Any) -> list[Any]:
                 try:
                     results.startTest(self.testcase_pre)
                     self.pre_process(response)
@@ -51,13 +49,10 @@ def wrapper(  # pylint: disable=inconsistent-return-statements
                     results.addError(self.testcase_pre, sys.exc_info())
                 else:
                     results.addSuccess(self.testcase_pre)
-                finally:
-                    cb_result = cb(response, **cb_kwargs)
-                    if isinstance(cb_result, (AsyncGenerator, CoroutineType)):
-                        raise TypeError("Contracts don't support async callbacks")
-                    return list(  # pylint: disable=return-in-finally
-                        cast(Iterable[Any], iterate_spider_output(cb_result))
-                    )
+                cb_result = cb(response, **cb_kwargs)
+                if isinstance(cb_result, (AsyncGenerator, CoroutineType)):
+                    raise TypeError("Contracts don't support async callbacks")
+                return list(cast(Iterable[Any], iterate_spider_output(cb_result)))
 
             request.callback = wrapper
 
@@ -69,9 +64,7 @@ def add_post_hook(self, request: Request, results: TestResult) -> Request:
             assert cb is not None
 
             @wraps(cb)
-            def wrapper(  # pylint: disable=inconsistent-return-statements
-                response: Response, **cb_kwargs: Any
-            ) -> list[Any]:
+            def wrapper(response: Response, **cb_kwargs: Any) -> list[Any]:
                 cb_result = cb(response, **cb_kwargs)
                 if isinstance(cb_result, (AsyncGenerator, CoroutineType)):
                     raise TypeError("Contracts don't support async callbacks")
@@ -86,8 +79,7 @@ def wrapper(  # pylint: disable=inconsistent-return-statements
                     results.addError(self.testcase_post, sys.exc_info())
                 else:
                     results.addSuccess(self.testcase_post)
-                finally:
-                    return output  # pylint: disable=return-in-finally
+                return output
 
             request.callback = wrapper
 

Test patch: diff --git a/tests/test_contracts.py b/tests/test_contracts.py
index d578b3af450..b0cb92d12d9 100644
--- a/tests/test_contracts.py
+++ b/tests/test_contracts.py
@@ -556,3 +556,61 @@ def test_inherited_contracts(self):
 
         requests = self.conman.from_spider(spider, self.results)
         self.assertTrue(requests)
+
+
+class CustomFailContractPreProcess(Contract):
+    name = "test_contract"
+
+    def pre_process(self, response):
+        raise KeyboardInterrupt("Pre-process exception")
+
+
+class CustomFailContractPostProcess(Contract):
+    name = "test_contract"
+
+    def post_process(self, response):
+        raise KeyboardInterrupt("Post-process exception")
+
+
+class CustomContractPrePostProcess(unittest.TestCase):
+
+    def setUp(self):
+        self.results = TextTestResult(stream=None, descriptions=False, verbosity=0)
+
+    def test_pre_hook_keyboard_interrupt(self):
+        spider = TestSpider()
+        response = ResponseMock()
+        contract = CustomFailContractPreProcess(spider.returns_request)
+        conman = ContractsManager([contract])
+
+        try:
+            request = conman.from_method(spider.returns_request, self.results)
+            contract.add_pre_hook(request, self.results)
+            # Expect this to raise a KeyboardInterrupt
+            request.callback(response, **request.cb_kwargs)
+        except KeyboardInterrupt as e:
+            self.assertEqual(str(e), "Pre-process exception")
+        else:
+            self.fail("KeyboardInterrupt not raised")
+
+        self.assertFalse(self.results.failures)
+        self.assertFalse(self.results.errors)
+
+    def test_post_hook_keyboard_interrupt(self):
+        spider = TestSpider()
+        response = ResponseMock()
+        contract = CustomFailContractPostProcess(spider.returns_request)
+        conman = ContractsManager([contract])
+
+        try:
+            request = conman.from_method(spider.returns_request, self.results)
+            contract.add_post_hook(request, self.results)
+            # Expect this to raise a KeyboardInterrupt
+            request.callback(response, **request.cb_kwargs)
+        except KeyboardInterrupt as e:
+            self.assertEqual(str(e), "Post-process exception")
+        else:
+            self.fail("KeyboardInterrupt not raised")
+
+        self.assertFalse(self.results.failures)
+        self.assertFalse(self.results.errors)