Repository: abhinavsingh/proxy.py
Pull request #899 (2021-12-21 23:09:39)

Raise `HttpProtocolException` instead of `ValueError` 
For clean teardown of the offending connection

codecov[bot]: # [Codecov](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) Report
> Merging [#899](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) (ebdd91a) into [develop](https://codecov.io/gh/abhinavsingh/proxy.py/commit/37c779ade9d06bc17d5f67fadec4ae72850d6799?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) (37c779a) will **decrease** coverage by `0.47%`.
> The diff coverage is `69.23%`.

[![Impacted file tree graph](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/graphs/tree.svg?width=650&height=150&src=pr&token=Zh9J7b4la2&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh)](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh)

```diff
@@             Coverage Diff             @@
##           develop     #899      +/-   ##
===========================================
- Coverage    87.02%   86.54%   -0.48%     
===========================================
  Files          130      130              
  Lines         5833     5843      +10     
  Branches       582      540      -42     
===========================================
- Hits          5076     5057      -19     
- Misses         652      669      +17     
- Partials       105      117      +12     
```

| Flag | Coverage Δ | |
|---|---|---|
| pytest | `86.54% <69.23%> (-0.32%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) | Coverage Δ | |
|---|---|---|
| [proxy/core/acceptor/threadless.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvY29yZS9hY2NlcHRvci90aHJlYWRsZXNzLnB5) | `84.69% <ø> (ø)` | |
| [proxy/http/exception/base.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC9leGNlcHRpb24vYmFzZS5weQ==) | `66.66% <50.00%> (-33.34%)` | :arrow_down: |
| [proxy/http/exception/http\_request\_rejected.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC9leGNlcHRpb24vaHR0cF9yZXF1ZXN0X3JlamVjdGVkLnB5) | `87.50% <50.00%> (-12.50%)` | :arrow_down: |
| [proxy/http/exception/proxy\_auth\_failed.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC9leGNlcHRpb24vcHJveHlfYXV0aF9mYWlsZWQucHk=) | `83.33% <50.00%> (-16.67%)` | :arrow_down: |
| [proxy/http/exception/proxy\_conn\_failed.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC9leGNlcHRpb24vcHJveHlfY29ubl9mYWlsZWQucHk=) | `87.50% <50.00%> (-12.50%)` | :arrow_down: |
| [proxy/http/handler.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC9oYW5kbGVyLnB5) | `71.49% <100.00%> (-0.91%)` | :arrow_down: |
| [proxy/http/parser/parser.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC9wYXJzZXIvcGFyc2VyLnB5) | `95.23% <100.00%> (+0.02%)` | :arrow_up: |
| [proxy/http/parser/protocol.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC9wYXJzZXIvcHJvdG9jb2wucHk=) | `100.00% <100.00%> (ø)` | |
| [tests/http/test\_http\_parser.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvaHR0cC90ZXN0X2h0dHBfcGFyc2VyLnB5) | `100.00% <100.00%> (ø)` | |
| [tests/http/test\_proxy\_protocol.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvaHR0cC90ZXN0X3Byb3h5X3Byb3RvY29sLnB5) | `96.07% <100.00%> (-3.93%)` | :arrow_down: |
| ... and [10 more](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh). Last update [37c779a...ebdd91a](https://codecov.io/gh/abhinavsingh/proxy.py/pull/899?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh).


Patch:
diff --git a/proxy/core/acceptor/threadless.py b/proxy/core/acceptor/threadless.py
index f36f9c0..cfd2f12 100644
--- a/proxy/core/acceptor/threadless.py
+++ b/proxy/core/acceptor/threadless.py
@@ -180,13 +180,15 @@ class Threadless(ABC, Generic[T]):
                 # else:
                 #     logger.info(
                 #         'fd#{0} by work#{1} not modified'.format(fileno, work_id))
+            # Can throw ValueError: Invalid file descriptor: -1
+            #
+            # A guard within Work classes may not help here due to
+            # asynchronous nature.  Hence, threadless will handle
+            # ValueError exceptions raised by selector.register
+            # for invalid fd.
+            #
+            # TODO: Also remove offending work from pool to avoid spin loop.
             elif fileno != -1:
-                # Can throw ValueError: Invalid file descriptor: -1
-                #
-                # A guard within Work classes may not help here due to
-                # asynchronous nature.  Hence, threadless will handle
-                # ValueError exceptions raised by selector.register
-                # for invalid fd.
                 self.selector.register(
                     fileno, events=mask,
                     data=work_id,
diff --git a/proxy/http/exception/base.py b/proxy/http/exception/base.py
index 37817c9..d39a640 100644
--- a/proxy/http/exception/base.py
+++ b/proxy/http/exception/base.py
@@ -12,9 +12,10 @@
 
        http
 """
-from typing import Optional
+from typing import Optional, TYPE_CHECKING
 
-from ..parser import HttpParser
+if TYPE_CHECKING:
+    from ..parser import HttpParser
 
 
 class HttpProtocolException(Exception):
@@ -25,5 +26,5 @@ class HttpProtocolException(Exception):
     ``response()`` method to optionally return custom response to client.
     """
 
-    def response(self, request: HttpParser) -> Optional[memoryview]:
+    def response(self, request: 'HttpParser') -> Optional[memoryview]:
         return None  # pragma: no cover
diff --git a/proxy/http/exception/http_request_rejected.py b/proxy/http/exception/http_request_rejected.py
index 1b3b1fd..6d09548 100644
--- a/proxy/http/exception/http_request_rejected.py
+++ b/proxy/http/exception/http_request_rejected.py
@@ -8,12 +8,15 @@
     :copyright: (c) 2013-present by Abhinav Singh and contributors.
     :license: BSD, see LICENSE for more details.
 """
-from typing import Optional, Dict
+from typing import Optional, Dict, TYPE_CHECKING
 
 from .base import HttpProtocolException
-from ..parser import HttpParser
+
 from ...common.utils import build_http_response
 
+if TYPE_CHECKING:
+    from ..parser import HttpParser
+
 
 class HttpRequestRejected(HttpProtocolException):
     """Generic exception that can be used to reject the client requests.
@@ -33,7 +36,7 @@ class HttpRequestRejected(HttpProtocolException):
         self.headers: Optional[Dict[bytes, bytes]] = headers
         self.body: Optional[bytes] = body
 
-    def response(self, _request: HttpParser) -> Optional[memoryview]:
+    def response(self, _request: 'HttpParser') -> Optional[memoryview]:
         if self.status_code:
             return memoryview(
                 build_http_response(
diff --git a/proxy/http/exception/proxy_auth_failed.py b/proxy/http/exception/proxy_auth_failed.py
index 6c96498..ecb9a29 100644
--- a/proxy/http/exception/proxy_auth_failed.py
+++ b/proxy/http/exception/proxy_auth_failed.py
@@ -13,14 +13,18 @@
        auth
        http
 """
+from typing import TYPE_CHECKING
+
 from .base import HttpProtocolException
 
 from ..codes import httpStatusCodes
-from ..parser import HttpParser
 
 from ...common.constants import PROXY_AGENT_HEADER_VALUE, PROXY_AGENT_HEADER_KEY
 from ...common.utils import build_http_response
 
+if TYPE_CHECKING:
+    from ..parser import HttpParser
+
 
 class ProxyAuthenticationFailed(HttpProtocolException):
     """Exception raised when HTTP Proxy auth is enabled and
@@ -39,5 +43,5 @@ class ProxyAuthenticationFailed(HttpProtocolException):
         ),
     )
 
-    def response(self, _request: HttpParser) -> memoryview:
+    def response(self, _request: 'HttpParser') -> memoryview:
         return self.RESPONSE_PKT
diff --git a/proxy/http/exception/proxy_conn_failed.py b/proxy/http/exception/proxy_conn_failed.py
index 3c90557..c091bf5 100644
--- a/proxy/http/exception/proxy_conn_failed.py
+++ b/proxy/http/exception/proxy_conn_failed.py
@@ -12,14 +12,18 @@
 
        conn
 """
+from typing import TYPE_CHECKING
+
 from .base import HttpProtocolException
 
 from ..codes import httpStatusCodes
-from ..parser import HttpParser
 
 from ...common.constants import PROXY_AGENT_HEADER_VALUE, PROXY_AGENT_HEADER_KEY
 from ...common.utils import build_http_response
 
+if TYPE_CHECKING:
+    from ..parser import HttpParser
+
 
 class ProxyConnectionFailed(HttpProtocolException):
     """Exception raised when ``HttpProxyPlugin`` is unable to establish connection to upstream server."""
@@ -41,5 +45,5 @@ class ProxyConnectionFailed(HttpProtocolException):
         self.port: int = port
         self.reason: str = reason
 
-    def response(self, _request: HttpParser) -> memoryview:
+    def response(self, _request: 'HttpParser') -> memoryview:
         return self.RESPONSE_PKT
diff --git a/proxy/http/handler.py b/proxy/http/handler.py
index 4a03aa5..c033e8b 100644
--- a/proxy/http/handler.py
+++ b/proxy/http/handler.py
@@ -237,7 +237,7 @@ class HttpProtocolHandler(BaseTcpServerHandler):
                         break
                     data = optional_data
         except HttpProtocolException as e:
-            logger.debug('HttpProtocolException raised')
+            logger.exception('HttpProtocolException raised', exc_info=e)
             response: Optional[memoryview] = e.response(self.request)
             if response:
                 self.work.queue(response)
diff --git a/proxy/http/parser/parser.py b/proxy/http/parser/parser.py
index c446b34..5ed7072 100644
--- a/proxy/http/parser/parser.py
+++ b/proxy/http/parser/parser.py
@@ -22,6 +22,7 @@ from ...common.flag import flags
 
 from ..url import Url
 from ..methods import httpMethods
+from ..exception import HttpProtocolException
 
 from .protocol import ProxyProtocol
 from .chunk import ChunkParser, chunkParserStates
@@ -363,10 +364,7 @@ class HttpParser:
                     break
                 # To avoid a possible attack vector, we raise exception
                 # if parser receives an invalid request line.
-                #
-                # TODO: Better to use raise HttpProtocolException,
-                # but we should solve circular import problem first.
-                raise ValueError('Invalid request line')
+                raise HttpProtocolException('Invalid request line %r' % raw)
             parts = line.split(WHITESPACE, 2)
             self.version = parts[0]
             self.code = parts[1]
diff --git a/proxy/http/parser/protocol.py b/proxy/http/parser/protocol.py
index d749fdc..c44192f 100644
--- a/proxy/http/parser/protocol.py
+++ b/proxy/http/parser/protocol.py
@@ -10,6 +10,8 @@
 """
 from typing import Optional, Tuple
 
+from ..exception import HttpProtocolException
+
 from ...common.constants import WHITESPACE
 
 PROXY_PROTOCOL_V2_SIGNATURE = b'\x0D\x0A\x0D\x0A\x00\x0D\x0A\x51\x55\x49\x54\x0A'
@@ -43,4 +45,6 @@ class ProxyProtocol:
             self.version = 2
             raise NotImplementedError()
         else:
-            raise ValueError('Neither a v1 or v2 proxy protocol packet')
+            raise HttpProtocolException(
+                'Neither a v1 or v2 proxy protocol packet',
+            )


Test patch:
diff --git a/.github/workflows/test-library.yml b/.github/workflows/test-library.yml
index 1b7928e..c344b67 100644
--- a/.github/workflows/test-library.yml
+++ b/.github/workflows/test-library.yml
@@ -668,12 +668,32 @@ jobs:
         make ca-certificates
         python3 -m proxy --version
 
+  check:  # This job does nothing and is only used for the branch protection
+    if: always()
+
+    needs:
+    - analyze
+    - test
+    - lint
+    - dashboard
+    - brew
+    - developer
+
+    runs-on: Ubuntu-latest
+
+    steps:
+    - name: Decide whether the needed jobs succeeded or failed
+      uses: re-actors/alls-green@release/v1
+      with:
+        jobs: ${{ toJSON(needs) }}
+
   docker:
     runs-on: Ubuntu-latest
     permissions:
       packages: write
+    if: success()
     needs:
-    - build
+    - check
     - pre-setup  # transitive, for accessing settings
     name: 🐳 containerize
     strategy:
@@ -786,26 +806,6 @@ jobs:
         }}'
         -t $CONTAINER_TAG .
 
-  check:  # This job does nothing and is only used for the branch protection
-    if: always()
-
-    needs:
-    - analyze
-    - test
-    - lint
-    - docker
-    - dashboard
-    - brew
-    - developer
-
-    runs-on: Ubuntu-latest
-
-    steps:
-    - name: Decide whether the needed jobs succeeded or failed
-      uses: re-actors/alls-green@release/v1
-      with:
-        jobs: ${{ toJSON(needs) }}
-
   publish-pypi:
     name: Publish 🐍📦 ${{ needs.pre-setup.outputs.git-tag }} to PyPI
     needs:
diff --git a/tests/http/test_http_parser.py b/tests/http/test_http_parser.py
index 774bf9f..5f771bd 100644
--- a/tests/http/test_http_parser.py
+++ b/tests/http/test_http_parser.py
@@ -15,6 +15,7 @@ from proxy.common.utils import build_http_request, build_http_response, build_ht
 from proxy.common.utils import find_http_line, bytes_
 
 from proxy.http import httpStatusCodes, httpMethods
+from proxy.http.exception import HttpProtocolException
 from proxy.http.parser import HttpParser, httpParserTypes, httpParserStates
 
 
@@ -24,10 +25,10 @@ class TestHttpParser(unittest.TestCase):
         self.parser = HttpParser(httpParserTypes.REQUEST_PARSER)
 
     def test_issue_127(self) -> None:
-        with self.assertRaises(ValueError):
+        with self.assertRaises(HttpProtocolException):
             self.parser.parse(CRLF)
 
-        with self.assertRaises(ValueError):
+        with self.assertRaises(HttpProtocolException):
             raw = b'qwqrqw!@!#@!#ad adfad\r\n'
             while True:
                 self.parser.parse(raw)
diff --git a/tests/http/test_proxy_protocol.py b/tests/http/test_proxy_protocol.py
index b6701ab..f8b609b 100644
--- a/tests/http/test_proxy_protocol.py
+++ b/tests/http/test_proxy_protocol.py
@@ -11,6 +11,7 @@
 import unittest
 
 from proxy.http.parser import ProxyProtocol, PROXY_PROTOCOL_V2_SIGNATURE
+from proxy.http.exception import HttpProtocolException
 
 
 class TestProxyProtocol(unittest.TestCase):
@@ -81,6 +82,6 @@ class TestProxyProtocol(unittest.TestCase):
             self.assertEqual(self.protocol.version, 2)
 
     def test_unknown_value_error(self) -> None:
-        with self.assertRaises(ValueError):
+        with self.assertRaises(HttpProtocolException):
             self.protocol.parse(PROXY_PROTOCOL_V2_SIGNATURE[:10])
             self.assertEqual(self.protocol.version, None)