Merge pull request #70 from clue-labs/no-master

Do not expose $master property and accept any ServerInterface for SecureServer

Author: WyriHaximus

README.md

@@ -295,14 +295,19 @@ Note that the `SecureServer` class is a concrete implementation for TLS sockets.
 If you want to typehint in your higher-level protocol implementation, you SHOULD
 use the generic [`ServerInterface`](#serverinterface) instead.
 
-> Advanced usage: Internally, the `SecureServer` has to set the required
-context options on the underlying stream resources.
-It should therefor be used with an unmodified `Server` instance as first
-parameter so that it can allocate an empty context resource which this
-class uses to set required TLS context options.
-Failing to do so may result in some hard to trace race conditions,
-because all stream resources will use a single, shared default context
-resource otherwise.
+> Advanced usage: Despite allowing any `ServerInterface` as first parameter,
+you SHOULD pass an unmodified `Server` instance as first parameter, unless you
+know what you're doing.
+Internally, the `SecureServer` has to set the required TLS context options on
+the underlying stream resources.
+These resources are not exposed through any of the interfaces defined in this
+package, but only through the `React\Stream\Stream` class.
+The unmodified `Server` class is guaranteed to emit connections that implement
+the `ConnectionInterface` and also extend the `Stream` class in order to
+expose these underlying resources.
+If you use a custom `ServerInterface` and its `connection` event does not
+meet this requirement, the `SecureServer` will emit an `error` event and
+then close the underlying connection.
 
 ### ConnectionInterface
 

src/SecureServer.php

@@ -56,6 +56,7 @@ class SecureServer extends EventEmitter implements ServerInterface
 {
     private $tcp;
     private $encryption;
+    private $context;
 
     /**
      * Creates a secure TLS server and starts waiting for incoming connections
@@ -94,39 +95,36 @@ class SecureServer extends EventEmitter implements ServerInterface
      * and/or PHP version.
      * Passing unknown context options has no effect.
      *
-     * Advanced usage: Internally, the `SecureServer` has to set the required
-     * context options on the underlying stream resources.
-     * It should therefor be used with an unmodified `Server` instance as first
-     * parameter so that it can allocate an empty context resource which this
-     * class uses to set required TLS context options.
-     * Failing to do so may result in some hard to trace race conditions,
-     * because all stream resources will use a single, shared default context
-     * resource otherwise.
+     * Advanced usage: Despite allowing any `ServerInterface` as first parameter,
+     * you SHOULD pass an unmodified `Server` instance as first parameter, unless you
+     * know what you're doing.
+     * Internally, the `SecureServer` has to set the required TLS context options on
+     * the underlying stream resources.
+     * These resources are not exposed through any of the interfaces defined in this
+     * package, but only through the `React\Stream\Stream` class.
+     * The unmodified `Server` class is guaranteed to emit connections that implement
+     * the `ConnectionInterface` and also extend the `Stream` class in order to
+     * expose these underlying resources.
+     * If you use a custom `ServerInterface` and its `connection` event does not
+     * meet this requirement, the `SecureServer` will emit an `error` event and
+     * then close the underlying connection.
      *
-     * @param Server $tcp
+     * @param ServerInterface|Server $tcp
      * @param LoopInterface $loop
      * @param array $context
-     * @throws ConnectionException
      * @see Server
      * @link http://php.net/manual/en/context.ssl.php for TLS context options
      */
-    public function __construct(Server $tcp, LoopInterface $loop, array $context)
+    public function __construct(ServerInterface $tcp, LoopInterface $loop, array $context)
     {
-        if (!is_resource($tcp->master)) {
-            throw new ConnectionException('TCP server already shut down');
-        }
-
         // default to empty passphrase to surpress blocking passphrase prompt
         $context += array(
             'passphrase' => ''
         );
 
-        foreach ($context as $name => $value) {
-            stream_context_set_option($tcp->master, 'ssl', $name, $value);
-        }
-
         $this->tcp = $tcp;
         $this->encryption = new StreamEncryption($loop);
+        $this->context = $context;
 
         $that = $this;
         $this->tcp->on('connection', function ($connection) use ($that) {
@@ -156,6 +154,10 @@ public function handleConnection(ConnectionInterface $connection)
             return;
         }
 
+        foreach ($this->context as $name => $value) {
+            stream_context_set_option($connection->stream, 'ssl', $name, $value);
+        }
+
         $that = $this;
 
         $this->encryption->enable($connection)->then(

src/Server.php

@@ -36,7 +36,7 @@
  */
 class Server extends EventEmitter implements ServerInterface
 {
-    public $master;
+    private $master;
     private $loop;
 
     /**

tests/FunctionalSecureServerTest.php

@@ -368,19 +368,6 @@ public function testEmitsErrorIfConnectionIsNotSecureHandshake()
         Block\sleep(self::TIMEOUT, $loop);
     }
 
-    /**
-     * @expectedException React\Socket\ConnectionException
-     */
-    public function testFailsToCreateSecureServerOnClosedSocket()
-    {
-        $loop = Factory::create();
-
-        $server = new Server(0, $loop);
-        $server->close();
-
-        new SecureServer($server, $loop, array());
-    }
-
     private function getPort(ServerInterface $server)
     {
         return parse_url($server->getAddress(), PHP_URL_PORT);

tests/FunctionalServerTest.php

@@ -229,7 +229,7 @@ public function testEmitsConnectionWithLocalIpv6()
         $this->assertEquals($server->getAddress(), $local);
     }
 
-    public function testAppliesContextOptionsToSocketStreamResource()
+    public function testEmitsConnectionWithInheritedContextOptions()
     {
         if (defined('HHVM_VERSION') && version_compare(HHVM_VERSION, '3.13', '<')) {
             // https://3v4l.org/hB4Tc
@@ -242,7 +242,18 @@ public function testAppliesContextOptionsToSocketStreamResource()
             'backlog' => 4
         ));
 
-        $all = stream_context_get_options($server->master);
+        $all = null;
+        $server->on('connection', function (ConnectionInterface $conn) use (&$all) {
+            $all = stream_context_get_options($conn->stream);
+        });
+        $port = $this->getPort($server);
+
+        $connector = new TcpConnector($loop);
+        $promise = $connector->create('127.0.0.1', $port);
+
+        $promise->then($this->expectCallableOnce());
+
+        Block\sleep(0.1, $loop);
 
         $this->assertEquals(array('socket' => array('backlog' => 4)), $all);
     }

tests/SecureServerTest.php

@@ -15,9 +15,8 @@ public function setUp()
 
     public function testGetAddressWillBePassedThroughToTcpServer()
     {
-        $tcp = $this->getMockBuilder('React\Socket\Server')->disableOriginalConstructor()->getMock();
+        $tcp = $this->getMockBuilder('React\Socket\ServerInterface')->getMock();
         $tcp->expects($this->once())->method('getAddress')->willReturn('127.0.0.1:1234');
-        $tcp->master = stream_socket_server('tcp://localhost:0');
 
         $loop = $this->getMock('React\EventLoop\LoopInterface');
 
@@ -28,9 +27,8 @@ public function testGetAddressWillBePassedThroughToTcpServer()
 
     public function testCloseWillBePassedThroughToTcpServer()
     {
-        $tcp = $this->getMockBuilder('React\Socket\Server')->disableOriginalConstructor()->getMock();
+        $tcp = $this->getMockBuilder('React\Socket\ServerInterface')->getMock();
         $tcp->expects($this->once())->method('close');
-        $tcp->master = stream_socket_server('tcp://localhost:0');
 
         $loop = $this->getMock('React\EventLoop\LoopInterface');
 
@@ -42,7 +40,6 @@ public function testCloseWillBePassedThroughToTcpServer()
     public function testConnectionWillBeEndedWithErrorIfItIsNotAStream()
     {
         $tcp = $this->getMockBuilder('React\Socket\Server')->disableOriginalConstructor()->setMethods(null)->getMock();
-        $tcp->master = stream_socket_server('tcp://localhost:0');
 
         $loop = $this->getMock('React\EventLoop\LoopInterface');
 
@@ -59,7 +56,6 @@ public function testConnectionWillBeEndedWithErrorIfItIsNotAStream()
     public function testSocketErrorWillBeForwarded()
     {
         $tcp = $this->getMockBuilder('React\Socket\Server')->disableOriginalConstructor()->setMethods(null)->getMock();
-        $tcp->master = stream_socket_server('tcp://localhost:0');
 
         $loop = $this->getMock('React\EventLoop\LoopInterface');