From c86fdadcdec7ad59724502ca4bd50d4cc4986980 Mon Sep 17 00:00:00 2001 From: Chris Boden Date: Tue, 3 Jun 2014 22:30:40 -0400 Subject: [PATCH 1/4] Revert "Fixed a memory leak when a connection is closed the topics should also be removed if they are empty" This reverts commit c089aea8ebad521795610feb36bc7a937794a88c. --- src/Ratchet/Wamp/TopicManager.php | 7 ++----- tests/unit/Wamp/TopicManagerTest.php | 7 ------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/src/Ratchet/Wamp/TopicManager.php b/src/Ratchet/Wamp/TopicManager.php index a69e315..bceee73 100644 --- a/src/Ratchet/Wamp/TopicManager.php +++ b/src/Ratchet/Wamp/TopicManager.php @@ -77,11 +77,8 @@ class TopicManager implements WsServerInterface, WampServerInterface { public function onClose(ConnectionInterface $conn) { $this->app->onClose($conn); - foreach ($this->topicLookup as $topic => $storage) { - $storage->remove($conn); - if (0 === $storage->count()) { - unset($this->topicLookup[$topic]); - } + foreach ($this->topicLookup as $topic) { + $topic->remove($conn); } } diff --git a/tests/unit/Wamp/TopicManagerTest.php b/tests/unit/Wamp/TopicManagerTest.php index 7439064..0c31d9a 100644 --- a/tests/unit/Wamp/TopicManagerTest.php +++ b/tests/unit/Wamp/TopicManagerTest.php @@ -166,19 +166,12 @@ class TopicManagerTest extends \PHPUnit_Framework_TestCase { $method = $class->getMethod('getTopic'); $method->setAccessible(true); - $attribute = $class->getProperty('topicLookup'); - $attribute->setAccessible(true); - $topic = $method->invokeArgs($this->mngr, array($name)); - $this->assertCount(1, $attribute->getValue($this->mngr)); - $this->mngr->onSubscribe($this->conn, $name); $this->mngr->onClose($this->conn); $this->assertFalse($topic->has($this->conn)); - - $this->assertCount(0, $attribute->getValue($this->mngr)); } public function testOnErrorBubbles() { From 87de418446e0c34fc923656ae140f2be5eddebdd Mon Sep 17 00:00:00 2001 From: Chris Boden Date: Wed, 4 Jun 2014 20:59:10 -0400 Subject: [PATCH 2/4] Revert "Revert "Fixed a memory leak when a connection is closed the topics should also be removed if they are empty"" This reverts commit c86fdadcdec7ad59724502ca4bd50d4cc4986980. --- src/Ratchet/Wamp/TopicManager.php | 7 +++++-- tests/unit/Wamp/TopicManagerTest.php | 7 +++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/Ratchet/Wamp/TopicManager.php b/src/Ratchet/Wamp/TopicManager.php index bceee73..a69e315 100644 --- a/src/Ratchet/Wamp/TopicManager.php +++ b/src/Ratchet/Wamp/TopicManager.php @@ -77,8 +77,11 @@ class TopicManager implements WsServerInterface, WampServerInterface { public function onClose(ConnectionInterface $conn) { $this->app->onClose($conn); - foreach ($this->topicLookup as $topic) { - $topic->remove($conn); + foreach ($this->topicLookup as $topic => $storage) { + $storage->remove($conn); + if (0 === $storage->count()) { + unset($this->topicLookup[$topic]); + } } } diff --git a/tests/unit/Wamp/TopicManagerTest.php b/tests/unit/Wamp/TopicManagerTest.php index 0c31d9a..7439064 100644 --- a/tests/unit/Wamp/TopicManagerTest.php +++ b/tests/unit/Wamp/TopicManagerTest.php @@ -166,12 +166,19 @@ class TopicManagerTest extends \PHPUnit_Framework_TestCase { $method = $class->getMethod('getTopic'); $method->setAccessible(true); + $attribute = $class->getProperty('topicLookup'); + $attribute->setAccessible(true); + $topic = $method->invokeArgs($this->mngr, array($name)); + $this->assertCount(1, $attribute->getValue($this->mngr)); + $this->mngr->onSubscribe($this->conn, $name); $this->mngr->onClose($this->conn); $this->assertFalse($topic->has($this->conn)); + + $this->assertCount(0, $attribute->getValue($this->mngr)); } public function testOnErrorBubbles() { From a0d858a638f89e2314343dad57f0618dda1f73c9 Mon Sep 17 00:00:00 2001 From: Chris Boden Date: Thu, 5 Jun 2014 08:13:35 -0400 Subject: [PATCH 3/4] [WAMP] Added autoDelete to Topics --- src/Ratchet/Wamp/Topic.php | 7 +++++++ src/Ratchet/Wamp/TopicManager.php | 26 +++++++++++++++++--------- tests/unit/Wamp/TopicManagerTest.php | 1 + 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/Ratchet/Wamp/Topic.php b/src/Ratchet/Wamp/Topic.php index f1bd68a..3fe73d1 100644 --- a/src/Ratchet/Wamp/Topic.php +++ b/src/Ratchet/Wamp/Topic.php @@ -6,6 +6,13 @@ use Ratchet\ConnectionInterface; * A topic/channel containing connections that have subscribed to it */ class Topic implements \IteratorAggregate, \Countable { + /** + * If true the TopicManager will destroy this object if it's ever empty of connections + * @deprecated in v0.4 + * @type bool + */ + public $autoDelete = false; + private $id; private $subscribers; diff --git a/src/Ratchet/Wamp/TopicManager.php b/src/Ratchet/Wamp/TopicManager.php index a69e315..318b986 100644 --- a/src/Ratchet/Wamp/TopicManager.php +++ b/src/Ratchet/Wamp/TopicManager.php @@ -54,13 +54,12 @@ class TopicManager implements WsServerInterface, WampServerInterface { public function onUnsubscribe(ConnectionInterface $conn, $topic) { $topicObj = $this->getTopic($topic); - if ($conn->WAMP->subscriptions->contains($topicObj)) { - $conn->WAMP->subscriptions->detach($topicObj); - } else { + if (!$conn->WAMP->subscriptions->contains($topicObj)) { return; } - $this->topicLookup[$topic]->remove($conn); + $this->cleanTopic($topicObj, $conn); + $this->app->onUnsubscribe($conn, $topicObj); } @@ -77,11 +76,8 @@ class TopicManager implements WsServerInterface, WampServerInterface { public function onClose(ConnectionInterface $conn) { $this->app->onClose($conn); - foreach ($this->topicLookup as $topic => $storage) { - $storage->remove($conn); - if (0 === $storage->count()) { - unset($this->topicLookup[$topic]); - } + foreach ($this->topicLookup as $topic) { + $this->cleanTopic($topic, $conn); } } @@ -114,4 +110,16 @@ class TopicManager implements WsServerInterface, WampServerInterface { return $this->topicLookup[$topic]; } + + protected function cleanTopic(Topic $topic, ConnectionInterface $conn) { + if ($conn->WAMP->subscriptions->contains($topic)) { + $conn->WAMP->subscriptions->detach($topic); + } + + $this->topicLookup[$topic->getId()]->remove($conn); + + if ($topic->autoDelete && 0 === $topic->count()) { + unset($this->topicLookup[$topic->getId()]); + } + } } diff --git a/tests/unit/Wamp/TopicManagerTest.php b/tests/unit/Wamp/TopicManagerTest.php index 7439064..79ff2ff 100644 --- a/tests/unit/Wamp/TopicManagerTest.php +++ b/tests/unit/Wamp/TopicManagerTest.php @@ -170,6 +170,7 @@ class TopicManagerTest extends \PHPUnit_Framework_TestCase { $attribute->setAccessible(true); $topic = $method->invokeArgs($this->mngr, array($name)); + $topic->autoDelete = true; $this->assertCount(1, $attribute->getValue($this->mngr)); From f21a1951c659455f91e4602b2235ff364b92f14a Mon Sep 17 00:00:00 2001 From: Chris Boden Date: Sat, 7 Jun 2014 10:51:32 -0400 Subject: [PATCH 4/4] [WAMP] Tests for new Topic autoDelete handling --- tests/unit/Wamp/TopicManagerTest.php | 35 ++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/tests/unit/Wamp/TopicManagerTest.php b/tests/unit/Wamp/TopicManagerTest.php index 79ff2ff..8482877 100644 --- a/tests/unit/Wamp/TopicManagerTest.php +++ b/tests/unit/Wamp/TopicManagerTest.php @@ -159,9 +159,7 @@ class TopicManagerTest extends \PHPUnit_Framework_TestCase { $this->mngr->onClose($this->conn); } - public function testConnIsRemovedFromTopicOnClose() { - $name = 'State testing'; - + protected function topicProvider($name) { $class = new \ReflectionClass('Ratchet\Wamp\TopicManager'); $method = $class->getMethod('getTopic'); $method->setAccessible(true); @@ -170,7 +168,13 @@ class TopicManagerTest extends \PHPUnit_Framework_TestCase { $attribute->setAccessible(true); $topic = $method->invokeArgs($this->mngr, array($name)); - $topic->autoDelete = true; + + return array($topic, $attribute); + } + + public function testConnIsRemovedFromTopicOnClose() { + $name = 'State Testing'; + list($topic, $attribute) = $this->topicProvider($name); $this->assertCount(1, $attribute->getValue($this->mngr)); @@ -178,8 +182,29 @@ class TopicManagerTest extends \PHPUnit_Framework_TestCase { $this->mngr->onClose($this->conn); $this->assertFalse($topic->has($this->conn)); + } - $this->assertCount(0, $attribute->getValue($this->mngr)); + public static function topicConnExpectationProvider() { + return array( + array(true, 'onClose', 0) + , array(true, 'onUnsubscribe', 0) + , array(false, 'onClose', 1) + , array(false, 'onUnsubscribe', 1) + ); + } + + /** + * @dataProvider topicConnExpectationProvider + */ + public function testTopicRetentionFromLeavingConnections($autoDelete, $methodCall, $expectation) { + $topicName = 'checkTopic'; + list($topic, $attribute) = $this->topicProvider($topicName); + $topic->autoDelete = $autoDelete; + + $this->mngr->onSubscribe($this->conn, $topicName); + call_user_func_array(array($this->mngr, $methodCall), array($this->conn, $topicName)); + + $this->assertCount($expectation, $attribute->getValue($this->mngr)); } public function testOnErrorBubbles() {