Repository: abhinavsingh/proxy.py
Pull request #708 (2021-11-08 16:34:11)

Move pid file write/remove within `AcceptorPool`

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

> :exclamation: Current head ad1b6cf differs from pull request most recent head 6c4b223. Consider uploading reports for the commit 6c4b223 to get more accurate results
[![Impacted file tree graph](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/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/708?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     #708      +/-   ##
===========================================
- Coverage    86.20%   85.96%   -0.24%     
===========================================
  Files          110      110              
  Lines         4929     4916      -13     
  Branches       404      404              
===========================================
- Hits          4249     4226      -23     
- Misses         604      607       +3     
- Partials        76       83       +7     
```

| Flag | Coverage Δ | |
|---|---|---|
| pytest | `85.96% <100.00%> (-0.10%)` | :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/708?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) | Coverage Δ | |
|---|---|---|
| [proxy/proxy.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvcHJveHkucHk=) | `100.00% <ø> (ø)` | |
| [tests/test\_main.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvdGVzdF9tYWluLnB5) | `99.40% <ø> (-0.60%)` | :arrow_down: |
| [proxy/core/acceptor/pool.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvY29yZS9hY2NlcHRvci9wb29sLnB5) | `86.66% <100.00%> (+1.66%)` | :arrow_up: |
| [tests/core/test\_acceptor\_pool.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvY29yZS90ZXN0X2FjY2VwdG9yX3Bvb2wucHk=) | `100.00% <100.00%> (ø)` | |
| [proxy/common/types.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvY29tbW9uL3R5cGVzLnB5) | `86.66% <0.00%> (-13.34%)` | :arrow_down: |
| [tests/test\_set\_open\_file\_limit.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvdGVzdF9zZXRfb3Blbl9maWxlX2xpbWl0LnB5) | `96.29% <0.00%> (-3.71%)` | :arrow_down: |
| [tests/testing/test\_embed.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvdGVzdGluZy90ZXN0X2VtYmVkLnB5) | `91.17% <0.00%> (-2.95%)` | :arrow_down: |
| [proxy/testing/test\_case.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvdGVzdGluZy90ZXN0X2Nhc2UucHk=) | `98.00% <0.00%> (-2.00%)` | :arrow_down: |
| [proxy/common/flag.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvY29tbW9uL2ZsYWcucHk=) | `93.16% <0.00%> (-1.71%)` | :arrow_down: |
| [proxy/common/utils.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvY29tbW9uL3V0aWxzLnB5) | `91.73% <0.00%> (-1.66%)` | :arrow_down: |
| ... and [3 more](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708/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/708?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/708?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh). Last update [db8da4f...6c4b223](https://codecov.io/gh/abhinavsingh/proxy.py/pull/708?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/README.md b/README.md
index 75ea1f5..16914c5 100644
--- a/README.md
+++ b/README.md
@@ -1780,18 +1780,31 @@ ssl.SSLEOFError: EOF occurred in violation of protocol (_ssl.c:997)
 
 ```console
                         +-------------+
+                        |             |
                         |  Proxy([])  |
+                        |             |
                         +------+------+
                                |
                                |
                    +-----------v--------------+
+                   |                          |
                    |    AcceptorPool(...)     |
+                   |                          |
                    +------------+-------------+
                                 |
-                                |
 +-----------------+             |           +-----------------+
+|                 |             |           |                 |
 |   Acceptor(..)  <-------------+----------->  Acceptor(..)   |
-+-----------------+                         +-----------------+
+|                 |                         |                 |
++---+-------------+                         +---------+-------+
+    |                                                 |
+    |                                                 |
+    |    +------++------++------++------++------+     |
+    |    |      ||      ||      ||      ||      |     |
+    +---->      ||      ||      ||      ||      <-----+
+         |      ||      ||      ||      ||      |
+         +------++------++------++------++------+
+                Threadless Worker Processes
 ```
 
 `proxy.py` is made with performance in mind.  By default, `proxy.py`
diff --git a/proxy/core/acceptor/pool.py b/proxy/core/acceptor/pool.py
index 3e16557..5684083 100644
--- a/proxy/core/acceptor/pool.py
+++ b/proxy/core/acceptor/pool.py
@@ -25,6 +25,7 @@ from .work import Work
 
 from ..event import EventQueue
 
+from ...common.utils import bytes_
 from ...common.flag import flags
 from ...common.constants import DEFAULT_BACKLOG, DEFAULT_IPV6_HOSTNAME
 from ...common.constants import DEFAULT_NUM_WORKERS, DEFAULT_PORT
@@ -126,6 +127,7 @@ class AcceptorPool:
 
     def setup(self) -> None:
         """Setup socket and acceptors."""
+        self._write_pid_file()
         if self.flags.unix_socket_path:
             self._listen_unix_socket()
         else:
@@ -155,6 +157,7 @@ class AcceptorPool:
             acceptor.join()
         if self.flags.unix_socket_path:
             os.remove(self.flags.unix_socket_path)
+        self._delete_pid_file()
         logger.debug('Acceptors shutdown')
 
     def _listen_unix_socket(self) -> None:
@@ -192,3 +195,14 @@ class AcceptorPool:
             self.acceptors.append(acceptor)
             self.work_queues.append(work_queue[0])
         logger.info('Started %d workers' % self.flags.num_workers)
+
+    def _write_pid_file(self) -> None:
+        if self.flags.pid_file is not None:
+            # NOTE: Multiple instances of proxy.py running on
+            # same host machine will currently result in overwriting the PID file
+            with open(self.flags.pid_file, 'wb') as pid_file:
+                pid_file.write(bytes_(os.getpid()))
+
+    def _delete_pid_file(self) -> None:
+        if self.flags.pid_file and os.path.exists(self.flags.pid_file):
+            os.remove(self.flags.pid_file)
diff --git a/proxy/proxy.py b/proxy/proxy.py
index 7059fd2..ced5c53 100644
--- a/proxy/proxy.py
+++ b/proxy/proxy.py
@@ -8,7 +8,6 @@
     :copyright: (c) 2013-present by Abhinav Singh and contributors.
     :license: BSD, see LICENSE for more details.
 """
-import os
 import sys
 import time
 import logging
@@ -18,7 +17,6 @@ from typing import List, Optional, Any, Type
 
 from proxy.core.acceptor.work import Work
 
-from .common.utils import bytes_
 from .core.acceptor import AcceptorPool
 from .http.handler import HttpProtocolHandler
 from .core.event import EventManager
@@ -93,15 +91,14 @@ flags.add_argument(
 
 
 class Proxy:
-    """Context manager to control core AcceptorPool server lifecycle.
+    """Context manager to control AcceptorPool & Eventing core lifecycle.
 
-    By default, AcceptorPool is started with HttpProtocolHandler worker class
-    i.e. we are only expecting HTTP traffic to flow between clients and server.
+    By default, AcceptorPool is started with `HttpProtocolHandler` work class.
+    By definition, it expects HTTP traffic to flow between clients and server.
 
-    Optionally, also initialize a global event queue.
-    It is a multiprocess safe queue which can be used to
-    build pubsub patterns for message sharing or signaling
-    within the running proxy environment.
+    Optionally, it also initializes the eventing core, a multiprocess safe
+    pubsub system queue which can be used to build various patterns
+    for message sharing and/or signaling.
     """
 
     def __init__(self, input_args: Optional[List[str]], **opts: Any) -> None:
@@ -113,17 +110,6 @@ class Proxy:
         self.work_klass: Type[Work] = HttpProtocolHandler
         self.event_manager: Optional[EventManager] = None
 
-    def write_pid_file(self) -> None:
-        if self.flags.pid_file is not None:
-            # TODO(abhinavsingh): Multiple instances of proxy.py running on
-            # same host machine will currently result in overwriting the PID file
-            with open(self.flags.pid_file, 'wb') as pid_file:
-                pid_file.write(bytes_(os.getpid()))
-
-    def delete_pid_file(self) -> None:
-        if self.flags.pid_file and os.path.exists(self.flags.pid_file):
-            os.remove(self.flags.pid_file)
-
     def __enter__(self) -> 'Proxy':
         if self.flags.enable_events:
             logger.info('Core Event enabled')
@@ -135,7 +121,6 @@ class Proxy:
             event_queue=self.event_manager.event_queue if self.event_manager is not None else None,
         )
         self.pool.setup()
-        self.write_pid_file()
         return self
 
     def __exit__(
@@ -149,7 +134,6 @@ class Proxy:
         if self.flags.enable_events:
             assert self.event_manager is not None
             self.event_manager.stop_event_dispatcher()
-        self.delete_pid_file()
 
 
 def main(


Test patch:
diff --git a/tests/core/test_acceptor_pool.py b/tests/core/test_acceptor_pool.py
index d84d3b2..b31e008 100644
--- a/tests/core/test_acceptor_pool.py
+++ b/tests/core/test_acceptor_pool.py
@@ -8,16 +8,23 @@
     :copyright: (c) 2013-present by Abhinav Singh and contributors.
     :license: BSD, see LICENSE for more details.
 """
-import unittest
+import os
 import socket
+import tempfile
+import unittest
+
 from unittest import mock
 
+from proxy.common.utils import bytes_
 from proxy.common.flag import FlagParser
 from proxy.core.acceptor import AcceptorPool
 
 
 class TestAcceptorPool(unittest.TestCase):
 
+    @mock.patch('os.remove')
+    @mock.patch('os.path.exists')
+    @mock.patch('builtins.open')
     @mock.patch('proxy.core.acceptor.pool.send_handle')
     @mock.patch('multiprocessing.Pipe')
     @mock.patch('socket.socket')
@@ -28,15 +35,19 @@ class TestAcceptorPool(unittest.TestCase):
             mock_socket: mock.Mock,
             mock_pipe: mock.Mock,
             mock_send_handle: mock.Mock,
+            mock_open: mock.Mock,
+            mock_exists: mock.Mock,
+            mock_remove: mock.Mock,
     ) -> None:
         acceptor1 = mock.MagicMock()
         acceptor2 = mock.MagicMock()
         mock_acceptor.side_effect = [acceptor1, acceptor2]
 
         num_workers = 2
+        pid_file = os.path.join(tempfile.gettempdir(), 'pid')
         sock = mock_socket.return_value
         work_klass = mock.MagicMock()
-        flags = FlagParser.initialize(num_workers=2)
+        flags = FlagParser.initialize(num_workers=2, pid_file=pid_file)
 
         pool = AcceptorPool(flags=flags, work_klass=work_klass)
         pool.setup()
@@ -66,5 +77,13 @@ class TestAcceptorPool(unittest.TestCase):
         sock.close.assert_called()
 
         pool.shutdown()
+
+        mock_open.assert_called_with(pid_file, 'wb')
+        mock_open.return_value.__enter__.return_value.write.assert_called_with(
+            bytes_(os.getpid()),
+        )
+        mock_exists.assert_called_with(pid_file)
+        mock_remove.assert_called_with(pid_file)
+
         acceptor1.join.assert_called()
         acceptor2.join.assert_called()
diff --git a/tests/test_main.py b/tests/test_main.py
index b9115cf..4948b86 100644
--- a/tests/test_main.py
+++ b/tests/test_main.py
@@ -9,8 +9,6 @@
     :license: BSD, see LICENSE for more details.
 """
 import unittest
-import tempfile
-import os
 
 from unittest import mock
 
@@ -208,40 +206,6 @@ class TestMain(unittest.TestCase):
         # Currently --enable-devtools alone doesn't enable eventing core
         mock_event_manager.assert_not_called()
 
-    @mock.patch('time.sleep')
-    @mock.patch('os.remove')
-    @mock.patch('os.path.exists')
-    @mock.patch('builtins.open')
-    @mock.patch('proxy.proxy.EventManager')
-    @mock.patch('proxy.proxy.AcceptorPool')
-    @mock.patch('proxy.common.flag.FlagParser.parse_args')
-    def test_pid_file_is_written_and_removed(
-            self,
-            mock_parse_args: mock.Mock,
-            mock_acceptor_pool: mock.Mock,
-            mock_event_manager: mock.Mock,
-            mock_open: mock.Mock,
-            mock_exists: mock.Mock,
-            mock_remove: mock.Mock,
-            mock_sleep: mock.Mock,
-    ) -> None:
-        pid_file = os.path.join(tempfile.gettempdir(), 'pid')
-        mock_sleep.side_effect = KeyboardInterrupt()
-        mock_args = mock_parse_args.return_value
-        self.mock_default_args(mock_args)
-        mock_args.pid_file = pid_file
-        main(['--pid-file', pid_file])
-        mock_parse_args.assert_called_once()
-        mock_acceptor_pool.assert_called()
-        mock_acceptor_pool.return_value.setup.assert_called()
-        mock_event_manager.assert_not_called()
-        mock_open.assert_called_with(pid_file, 'wb')
-        mock_open.return_value.__enter__.return_value.write.assert_called_with(
-            bytes_(os.getpid()),
-        )
-        mock_exists.assert_called_with(pid_file)
-        mock_remove.assert_called_with(pid_file)
-
     @mock.patch('time.sleep')
     @mock.patch('proxy.proxy.EventManager')
     @mock.patch('proxy.proxy.AcceptorPool')