Browse Source

Change force_new semantics to not evict existing channels

Stanley Cheung 8 years ago
parent
commit
1ad44c0fde
2 changed files with 11 additions and 16 deletions
  1. 3 9
      src/php/ext/grpc/channel.c
  2. 8 7
      src/php/tests/unit_tests/ChannelTest.php

+ 3 - 9
src/php/ext/grpc/channel.c

@@ -203,10 +203,8 @@ void create_and_add_channel_to_persistent_list(
  * credentials.
  *
  * If the $args array contains a "force_new" key mapping to a boolean value
- * of "true", a new underlying grpc_channel will be created regardless. If
- * there are any opened channels on the same hostname, user must manually
- * call close() on those dangling channels before the end of the PHP
- * script.
+ * of "true", a new and separate underlying grpc_channel will be created
+ * regardless. This will not affect existing channels.
  *
  * @param string $target The hostname to associate with this channel
  * @param array $args_array The arguments to pass to the Channel
@@ -297,11 +295,7 @@ PHP_METHOD(Channel, __construct) {
   gpr_mu_init(&channel->wrapper->mu);
   smart_str_free(&buf);
 
-  if (force_new) {
-    php_grpc_delete_persistent_list_entry(key, key_len TSRMLS_CC);
-  }
-
-  if (creds != NULL && creds->has_call_creds) {
+  if (force_new || (creds != NULL && creds->has_call_creds)) {
     // If the ChannelCredentials object was composed with a CallCredentials
     // object, there is no way we can tell them apart. Do NOT persist
     // them. They should be individually destroyed.

+ 8 - 7
src/php/tests/unit_tests/ChannelTest.php

@@ -532,8 +532,6 @@ class ChannelTest extends PHPUnit_Framework_TestCase
         $state = $this->channel2->getConnectivityState();
         $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
 
-        // any dangling old connection to the same host must be
-        // manually closed
         $this->channel1->close();
         $this->channel2->close();
     }
@@ -544,6 +542,7 @@ class ChannelTest extends PHPUnit_Framework_TestCase
         $this->channel1 = new Grpc\Channel('localhost:1', []);
         $this->channel2 = new Grpc\Channel('localhost:1',
                                            ["force_new" => true]);
+        // channel3 shares with channel1
         $this->channel3 = new Grpc\Channel('localhost:1', []);
 
         // try to connect on channel2
@@ -555,29 +554,31 @@ class ChannelTest extends PHPUnit_Framework_TestCase
         $state = $this->channel2->getConnectivityState();
         $this->assertConnecting($state);
         $state = $this->channel3->getConnectivityState();
-        $this->assertConnecting($state);
+        $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
 
         $this->channel1->close();
         $this->channel2->close();
     }
 
+    /**
+     * @expectedException RuntimeException
+     */
     public function testPersistentChannelForceNewOldChannelClose()
     {
 
         $this->channel1 = new Grpc\Channel('localhost:1', []);
         $this->channel2 = new Grpc\Channel('localhost:1',
                                            ["force_new" => true]);
+        // channel3 shares with channel1
         $this->channel3 = new Grpc\Channel('localhost:1', []);
 
         $this->channel1->close();
 
         $state = $this->channel2->getConnectivityState();
         $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
-        $state = $this->channel3->getConnectivityState();
-        $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
 
-        $this->channel2->close();
-        $this->channel3->close();
+        // channel3 already closed
+        $state = $this->channel3->getConnectivityState();
     }
 
     public function testPersistentChannelForceNewNewChannelClose()