Move pid file write/remove within AcceptorPool (#708)

* Move pid file write/remove within AcceptorPool

* Remove unused

Author: abhinavsingh

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`

proxy/core/acceptor/pool.py

@@ -25,6 +25,7 @@
 
 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 @@ def __exit__(
 
     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 @@ def shutdown(self) -> None:
             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 @@ def _start_acceptors(self) -> None:
             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)

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 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 @@
 
 
 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 @@ def __init__(self, input_args: Optional[List[str]], **opts: Any) -> None:
         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 @@ def __enter__(self) -> '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 @@ def __exit__(
         if self.flags.enable_events:
             assert self.event_manager is not None
             self.event_manager.stop_event_dispatcher()
-        self.delete_pid_file()
 
 
 def main(

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 @@ def test_setup_and_shutdown(
             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 @@ def test_setup_and_shutdown(
         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()

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 @@ def test_enable_devtools(
         # 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')