Merge pull request #14114 from jtattermusch/bump_190_pre2
1.9.0-pre1 is now 1.9.0-pre2
diff --git a/package.xml b/package.xml
index 24d9503..43f36a8 100644
--- a/package.xml
+++ b/package.xml
@@ -10,7 +10,7 @@
<email>grpc-packages@google.com</email>
<active>yes</active>
</lead>
- <date>2017-08-24</date>
+ <date>2018-01-23</date>
<time>16:06:07</time>
<version>
<release>1.9.0RC2</release>
@@ -22,12 +22,10 @@
</stability>
<license>Apache 2.0</license>
<notes>
-- Channel are now by default persistent #11878
-- Some bug fixes from 1.4 branch #12109, #12123
-- Fixed hang bug when fork() was used #11814
-- License changed to Apache 2.0
-- Added support for php_namespace option in codegen plugin #11886
-- Updated gRPC C Core library version 1.6
+- Updated gRPC C Core library version 1.9
+- Report grpc extension version in phpinfo() #13687
+- Fixed memory leak when handling metadata array #13660
+- Fixed memory involing persistent channels #14125 - #14130
</notes>
<contents>
<dir baseinstalldir="/" name="/">
diff --git a/src/core/ext/filters/max_age/max_age_filter.cc b/src/core/ext/filters/max_age/max_age_filter.cc
index 7b86e4c..a3f9780 100644
--- a/src/core/ext/filters/max_age/max_age_filter.cc
+++ b/src/core/ext/filters/max_age/max_age_filter.cc
@@ -37,6 +37,12 @@
#define MAX_CONNECTION_IDLE_INTEGER_OPTIONS \
{ DEFAULT_MAX_CONNECTION_IDLE_MS, 1, INT_MAX }
+/* States for idle_state in channel_data */
+#define MAX_IDLE_STATE_INIT ((gpr_atm)0)
+#define MAX_IDLE_STATE_SEEN_EXIT_IDLE ((gpr_atm)1)
+#define MAX_IDLE_STATE_SEEN_ENTER_IDLE ((gpr_atm)2)
+#define MAX_IDLE_STATE_TIMER_SET ((gpr_atm)3)
+
namespace {
struct channel_data {
/* We take a reference to the channel stack for the timer callback */
@@ -64,7 +70,7 @@
grpc_millis max_connection_age_grace;
/* Closure to run when the channel's idle duration reaches max_connection_idle
and should be closed gracefully */
- grpc_closure close_max_idle_channel;
+ grpc_closure max_idle_timer_cb;
/* Closure to run when the channel reaches its max age and should be closed
gracefully */
grpc_closure close_max_age_channel;
@@ -85,26 +91,117 @@
grpc_connectivity_state connectivity_state;
/* Number of active calls */
gpr_atm call_count;
+ /* TODO(zyc): C++lize this state machine */
+ /* 'idle_state' holds the states of max_idle_timer and channel idleness.
+ It can contain one of the following values:
+ +--------------------------------+----------------+---------+
+ | idle_state | max_idle_timer | channel |
+ +--------------------------------+----------------+---------+
+ | MAX_IDLE_STATE_INIT | unset | busy |
+ | MAX_IDLE_STATE_TIMER_SET | set, valid | idle |
+ | MAX_IDLE_STATE_SEEN_EXIT_IDLE | set, invalid | busy |
+ | MAX_IDLE_STATE_SEEN_ENTER_IDLE | set, invalid | idle |
+ +--------------------------------+----------------+---------+
+
+ MAX_IDLE_STATE_INIT: The initial and final state of 'idle_state'. The
+ channel has 1 or 1+ active calls, and the the timer is not set. Note that
+ we may put a virtual call to hold this state at channel initialization or
+ shutdown, so that the channel won't enter other states.
+
+ MAX_IDLE_STATE_TIMER_SET: The state after the timer is set and no calls
+ have arrived after the timer is set. The channel must have 0 active call in
+ this state. If the timer is fired in this state, we will close the channel
+ due to idleness.
+
+ MAX_IDLE_STATE_SEEN_EXIT_IDLE: The state after the timer is set and at
+ least one call has arrived after the timer is set. The channel must have 1
+ or 1+ active calls in this state. If the timer is fired in this state, we
+ won't reschudle it.
+
+ MAX_IDLE_STATE_SEEN_ENTER_IDLE: The state after the timer is set and the at
+ least one call has arrived after the timer is set, BUT the channel
+ currently has 1 or 1+ active calls. If the timer is fired in this state, we
+ will reschudle it.
+
+ max_idle_timer will not be cancelled (unless the channel is shutting down).
+ If the timer callback is called when the max_idle_timer is valid (i.e.
+ idle_state is MAX_IDLE_STATE_TIMER_SET), the channel will be closed due to
+ idleness, otherwise the channel won't be changed.
+
+ State transitions:
+ MAX_IDLE_STATE_INIT <-------3------ MAX_IDLE_STATE_SEEN_EXIT_IDLE
+ ^ | ^ ^ |
+ | | | | |
+ 1 2 +-----------4------------+ 6 7
+ | | | | |
+ | v | | v
+ MAX_IDLE_STATE_TIMER_SET <----5------ MAX_IDLE_STATE_SEEN_ENTER_IDLE
+
+ For 1, 3, 5 : See max_idle_timer_cb() function
+ For 2, 7 : See decrease_call_count() function
+ For 4, 6 : See increase_call_count() function */
+ gpr_atm idle_state;
+ /* Time when the channel finished its last outstanding call, in grpc_millis */
+ gpr_atm last_enter_idle_time_millis;
};
} // namespace
/* Increase the nubmer of active calls. Before the increasement, if there are no
calls, the max_idle_timer should be cancelled. */
static void increase_call_count(channel_data* chand) {
+ /* Exit idle */
if (gpr_atm_full_fetch_add(&chand->call_count, 1) == 0) {
- grpc_timer_cancel(&chand->max_idle_timer);
+ while (true) {
+ gpr_atm idle_state = gpr_atm_acq_load(&chand->idle_state);
+ switch (idle_state) {
+ case MAX_IDLE_STATE_TIMER_SET:
+ /* max_idle_timer_cb may have already set idle_state to
+ MAX_IDLE_STATE_INIT, in this case, we don't need to set it to
+ MAX_IDLE_STATE_SEEN_EXIT_IDLE */
+ gpr_atm_rel_cas(&chand->idle_state, MAX_IDLE_STATE_TIMER_SET,
+ MAX_IDLE_STATE_SEEN_EXIT_IDLE);
+ return;
+ case MAX_IDLE_STATE_SEEN_ENTER_IDLE:
+ gpr_atm_rel_store(&chand->idle_state, MAX_IDLE_STATE_SEEN_EXIT_IDLE);
+ return;
+ default:
+ /* try again */
+ break;
+ }
+ }
}
}
/* Decrease the nubmer of active calls. After the decrement, if there are no
calls, the max_idle_timer should be started. */
static void decrease_call_count(channel_data* chand) {
+ /* Enter idle */
if (gpr_atm_full_fetch_add(&chand->call_count, -1) == 1) {
- GRPC_CHANNEL_STACK_REF(chand->channel_stack, "max_age max_idle_timer");
- grpc_timer_init(
- &chand->max_idle_timer,
- grpc_core::ExecCtx::Get()->Now() + chand->max_connection_idle,
- &chand->close_max_idle_channel);
+ gpr_atm_no_barrier_store(&chand->last_enter_idle_time_millis,
+ (gpr_atm)grpc_core::ExecCtx::Get()->Now());
+ while (true) {
+ gpr_atm idle_state = gpr_atm_acq_load(&chand->idle_state);
+ switch (idle_state) {
+ case MAX_IDLE_STATE_INIT:
+ GRPC_CHANNEL_STACK_REF(chand->channel_stack,
+ "max_age max_idle_timer");
+ grpc_timer_init(
+ &chand->max_idle_timer,
+ grpc_core::ExecCtx::Get()->Now() + chand->max_connection_idle,
+ &chand->max_idle_timer_cb);
+ gpr_atm_rel_store(&chand->idle_state, MAX_IDLE_STATE_TIMER_SET);
+ return;
+ case MAX_IDLE_STATE_SEEN_EXIT_IDLE:
+ if (gpr_atm_rel_cas(&chand->idle_state, MAX_IDLE_STATE_SEEN_EXIT_IDLE,
+ MAX_IDLE_STATE_SEEN_ENTER_IDLE)) {
+ return;
+ }
+ break;
+ default:
+ /* try again */
+ break;
+ }
+ }
}
}
@@ -152,20 +249,58 @@
"max_age start_max_age_grace_timer_after_goaway_op");
}
-static void close_max_idle_channel(void* arg, grpc_error* error) {
+static void close_max_idle_channel(channel_data* chand) {
+ /* Prevent the max idle timer from being set again */
+ gpr_atm_no_barrier_fetch_add(&chand->call_count, 1);
+ grpc_transport_op* op = grpc_make_transport_op(nullptr);
+ op->goaway_error =
+ grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING("max_idle"),
+ GRPC_ERROR_INT_HTTP2_ERROR, GRPC_HTTP2_NO_ERROR);
+ grpc_channel_element* elem =
+ grpc_channel_stack_element(chand->channel_stack, 0);
+ elem->filter->start_transport_op(elem, op);
+}
+
+static void max_idle_timer_cb(void* arg, grpc_error* error) {
channel_data* chand = (channel_data*)arg;
if (error == GRPC_ERROR_NONE) {
- /* Prevent the max idle timer from being set again */
- gpr_atm_no_barrier_fetch_add(&chand->call_count, 1);
- grpc_transport_op* op = grpc_make_transport_op(nullptr);
- op->goaway_error =
- grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING("max_idle"),
- GRPC_ERROR_INT_HTTP2_ERROR, GRPC_HTTP2_NO_ERROR);
- grpc_channel_element* elem =
- grpc_channel_stack_element(chand->channel_stack, 0);
- elem->filter->start_transport_op(elem, op);
- } else if (error != GRPC_ERROR_CANCELLED) {
- GRPC_LOG_IF_ERROR("close_max_idle_channel", error);
+ bool try_again = true;
+ while (try_again) {
+ gpr_atm idle_state = gpr_atm_acq_load(&chand->idle_state);
+ switch (idle_state) {
+ case MAX_IDLE_STATE_TIMER_SET:
+ close_max_idle_channel(chand);
+ /* This MAX_IDLE_STATE_INIT is a final state, we don't have to check
+ * if idle_state has been changed */
+ gpr_atm_rel_store(&chand->idle_state, MAX_IDLE_STATE_INIT);
+ try_again = false;
+ break;
+ case MAX_IDLE_STATE_SEEN_EXIT_IDLE:
+ if (gpr_atm_rel_cas(&chand->idle_state, MAX_IDLE_STATE_SEEN_EXIT_IDLE,
+ MAX_IDLE_STATE_INIT)) {
+ try_again = false;
+ }
+ break;
+ case MAX_IDLE_STATE_SEEN_ENTER_IDLE:
+ GRPC_CHANNEL_STACK_REF(chand->channel_stack,
+ "max_age max_idle_timer");
+ grpc_timer_init(&chand->max_idle_timer,
+ (grpc_millis)gpr_atm_no_barrier_load(
+ &chand->last_enter_idle_time_millis) +
+ chand->max_connection_idle,
+ &chand->max_idle_timer_cb);
+ /* idle_state may have already been set to
+ MAX_IDLE_STATE_SEEN_EXIT_IDLE by increase_call_count(), in this
+ case, we don't need to set it to MAX_IDLE_STATE_TIMER_SET */
+ gpr_atm_rel_cas(&chand->idle_state, MAX_IDLE_STATE_SEEN_ENTER_IDLE,
+ MAX_IDLE_STATE_TIMER_SET);
+ try_again = false;
+ break;
+ default:
+ /* try again */
+ break;
+ }
+ }
}
GRPC_CHANNEL_STACK_UNREF(chand->channel_stack, "max_age max_idle_timer");
}
@@ -288,6 +423,9 @@
chand->max_connection_idle = DEFAULT_MAX_CONNECTION_IDLE_MS == INT_MAX
? GRPC_MILLIS_INF_FUTURE
: DEFAULT_MAX_CONNECTION_IDLE_MS;
+ chand->idle_state = MAX_IDLE_STATE_INIT;
+ gpr_atm_no_barrier_store(&chand->last_enter_idle_time_millis,
+ GRPC_MILLIS_INF_PAST);
for (size_t i = 0; i < args->channel_args->num_args; ++i) {
if (0 == strcmp(args->channel_args->args[i].key,
GRPC_ARG_MAX_CONNECTION_AGE_MS)) {
@@ -311,8 +449,8 @@
value == INT_MAX ? GRPC_MILLIS_INF_FUTURE : value;
}
}
- GRPC_CLOSURE_INIT(&chand->close_max_idle_channel, close_max_idle_channel,
- chand, grpc_schedule_on_exec_ctx);
+ GRPC_CLOSURE_INIT(&chand->max_idle_timer_cb, max_idle_timer_cb, chand,
+ grpc_schedule_on_exec_ctx);
GRPC_CLOSURE_INIT(&chand->close_max_age_channel, close_max_age_channel, chand,
grpc_schedule_on_exec_ctx);
GRPC_CLOSURE_INIT(&chand->force_close_max_age_channel,
diff --git a/src/csharp/Grpc.Core.Tests/Internal/DefaultObjectPoolTest.cs b/src/csharp/Grpc.Core.Tests/Internal/DefaultObjectPoolTest.cs
index b6bb0a9..9c6f8a2 100644
--- a/src/csharp/Grpc.Core.Tests/Internal/DefaultObjectPoolTest.cs
+++ b/src/csharp/Grpc.Core.Tests/Internal/DefaultObjectPoolTest.cs
@@ -60,6 +60,16 @@
}
[Test]
+ public void LeaseSetsReturnAction()
+ {
+ var pool = new DefaultObjectPool<TestPooledObject>(() => new TestPooledObject(), 10, 0);
+ var origLeased = pool.Lease();
+ origLeased.ReturnAction(origLeased);
+ pool.Dispose();
+ Assert.AreNotSame(origLeased, pool.Lease());
+ }
+
+ [Test]
public void Constructor()
{
Assert.Throws<ArgumentNullException>(() => new DefaultObjectPool<TestPooledObject>(null, 10, 2));
@@ -67,8 +77,14 @@
Assert.Throws<ArgumentException>(() => new DefaultObjectPool<TestPooledObject>(() => new TestPooledObject(), 10, -1));
}
- class TestPooledObject : IDisposable
+ class TestPooledObject : IPooledObject<TestPooledObject>
{
+ public Action<TestPooledObject> ReturnAction;
+
+ public void SetReturnToPoolAction(Action<TestPooledObject> returnAction)
+ {
+ this.ReturnAction = returnAction;
+ }
public void Dispose()
{
diff --git a/src/csharp/Grpc.Core/GrpcEnvironment.cs b/src/csharp/Grpc.Core/GrpcEnvironment.cs
index 6296f18..6bb2f6c 100644
--- a/src/csharp/Grpc.Core/GrpcEnvironment.cs
+++ b/src/csharp/Grpc.Core/GrpcEnvironment.cs
@@ -299,8 +299,8 @@
private GrpcEnvironment()
{
GrpcNativeInit();
- batchContextPool = new DefaultObjectPool<BatchContextSafeHandle>(() => BatchContextSafeHandle.Create(this.batchContextPool), batchContextPoolSharedCapacity, batchContextPoolThreadLocalCapacity);
- requestCallContextPool = new DefaultObjectPool<RequestCallContextSafeHandle>(() => RequestCallContextSafeHandle.Create(this.requestCallContextPool), requestCallContextPoolSharedCapacity, requestCallContextPoolThreadLocalCapacity);
+ batchContextPool = new DefaultObjectPool<BatchContextSafeHandle>(() => BatchContextSafeHandle.Create(), batchContextPoolSharedCapacity, batchContextPoolThreadLocalCapacity);
+ requestCallContextPool = new DefaultObjectPool<RequestCallContextSafeHandle>(() => RequestCallContextSafeHandle.Create(), requestCallContextPoolSharedCapacity, requestCallContextPoolThreadLocalCapacity);
threadPool = new GrpcThreadPool(this, GetThreadPoolSizeOrDefault(), GetCompletionQueueCountOrDefault(), inlineHandlers);
threadPool.Start();
}
diff --git a/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs b/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs
index 83385ad..53a859d 100644
--- a/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs
+++ b/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs
@@ -33,22 +33,21 @@
/// <summary>
/// grpcsharp_batch_context
/// </summary>
- internal class BatchContextSafeHandle : SafeHandleZeroIsInvalid, IOpCompletionCallback
+ internal class BatchContextSafeHandle : SafeHandleZeroIsInvalid, IOpCompletionCallback, IPooledObject<BatchContextSafeHandle>
{
static readonly NativeMethods Native = NativeMethods.Get();
static readonly ILogger Logger = GrpcEnvironment.Logger.ForType<BatchContextSafeHandle>();
- IObjectPool<BatchContextSafeHandle> ownedByPool;
+ Action<BatchContextSafeHandle> returnToPoolAction;
CompletionCallbackData completionCallbackData;
private BatchContextSafeHandle()
{
}
- public static BatchContextSafeHandle Create(IObjectPool<BatchContextSafeHandle> ownedByPool = null)
+ public static BatchContextSafeHandle Create()
{
var ctx = Native.grpcsharp_batch_context_create();
- ctx.ownedByPool = ownedByPool;
return ctx;
}
@@ -60,6 +59,12 @@
}
}
+ public void SetReturnToPoolAction(Action<BatchContextSafeHandle> returnAction)
+ {
+ GrpcPreconditions.CheckState(returnToPoolAction == null);
+ returnToPoolAction = returnAction;
+ }
+
public void SetCompletionCallback(BatchCompletionDelegate callback, object state)
{
GrpcPreconditions.CheckState(completionCallbackData.Callback == null);
@@ -109,10 +114,15 @@
public void Recycle()
{
- if (ownedByPool != null)
+ if (returnToPoolAction != null)
{
Native.grpcsharp_batch_context_reset(this);
- ownedByPool.Return(this);
+
+ var origReturnAction = returnToPoolAction;
+ // Not clearing all the references to the pool could prevent garbage collection of the pool object
+ // and thus cause memory leaks.
+ returnToPoolAction = null;
+ origReturnAction(this);
}
else
{
diff --git a/src/csharp/Grpc.Core/Internal/DefaultObjectPool.cs b/src/csharp/Grpc.Core/Internal/DefaultObjectPool.cs
index 2f030f3..0e1dc4d 100644
--- a/src/csharp/Grpc.Core/Internal/DefaultObjectPool.cs
+++ b/src/csharp/Grpc.Core/Internal/DefaultObjectPool.cs
@@ -27,9 +27,10 @@
/// Pool of objects that combines a shared pool and a thread local pool.
/// </summary>
internal class DefaultObjectPool<T> : IObjectPool<T>
- where T : class, IDisposable
+ where T : class, IPooledObject<T>
{
readonly object myLock = new object();
+ readonly Action<T> returnAction;
readonly Func<T> itemFactory;
// Queue shared between threads, access needs to be synchronized.
@@ -54,6 +55,7 @@
{
GrpcPreconditions.CheckArgument(sharedCapacity >= 0);
GrpcPreconditions.CheckArgument(threadLocalCapacity >= 0);
+ this.returnAction = Return;
this.itemFactory = GrpcPreconditions.CheckNotNull(itemFactory, nameof(itemFactory));
this.sharedQueue = new Queue<T>(sharedCapacity);
this.sharedCapacity = sharedCapacity;
@@ -74,6 +76,13 @@
/// </summary>
public T Lease()
{
+ var item = LeaseInternal();
+ item.SetReturnToPoolAction(returnAction);
+ return item;
+ }
+
+ private T LeaseInternal()
+ {
var localData = threadLocalData.Value;
if (localData.Queue.Count > 0)
{
diff --git a/src/csharp/Grpc.Core/Internal/IPooledObject.cs b/src/csharp/Grpc.Core/Internal/IPooledObject.cs
new file mode 100644
index 0000000..e20bd51
--- /dev/null
+++ b/src/csharp/Grpc.Core/Internal/IPooledObject.cs
@@ -0,0 +1,34 @@
+#region Copyright notice and license
+
+// Copyright 2018 gRPC authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#endregion
+
+using System;
+
+namespace Grpc.Core.Internal
+{
+ /// <summary>
+ /// An object that can be pooled in <c>IObjectPool</c>.
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ internal interface IPooledObject<T> : IDisposable
+ {
+ /// <summary>
+ /// Set the action that will be invoked to return a leased object to the pool.
+ /// </summary>
+ void SetReturnToPoolAction(Action<T> returnAction);
+ }
+}
diff --git a/src/csharp/Grpc.Core/Internal/RequestCallContextSafeHandle.cs b/src/csharp/Grpc.Core/Internal/RequestCallContextSafeHandle.cs
index 59e9d9b..ebc2d6d 100644
--- a/src/csharp/Grpc.Core/Internal/RequestCallContextSafeHandle.cs
+++ b/src/csharp/Grpc.Core/Internal/RequestCallContextSafeHandle.cs
@@ -20,26 +20,26 @@
using System.Runtime.InteropServices;
using Grpc.Core;
using Grpc.Core.Logging;
+using Grpc.Core.Utils;
namespace Grpc.Core.Internal
{
/// <summary>
/// grpcsharp_request_call_context
/// </summary>
- internal class RequestCallContextSafeHandle : SafeHandleZeroIsInvalid, IOpCompletionCallback
+ internal class RequestCallContextSafeHandle : SafeHandleZeroIsInvalid, IOpCompletionCallback, IPooledObject<RequestCallContextSafeHandle>
{
static readonly NativeMethods Native = NativeMethods.Get();
static readonly ILogger Logger = GrpcEnvironment.Logger.ForType<RequestCallContextSafeHandle>();
- IObjectPool<RequestCallContextSafeHandle> ownedByPool;
+ Action<RequestCallContextSafeHandle> returnToPoolAction;
private RequestCallContextSafeHandle()
{
}
- public static RequestCallContextSafeHandle Create(IObjectPool<RequestCallContextSafeHandle> ownedByPool = null)
+ public static RequestCallContextSafeHandle Create()
{
var ctx = Native.grpcsharp_request_call_context_create();
- ctx.ownedByPool = ownedByPool;
return ctx;
}
@@ -51,6 +51,12 @@
}
}
+ public void SetReturnToPoolAction(Action<RequestCallContextSafeHandle> returnAction)
+ {
+ GrpcPreconditions.CheckState(returnToPoolAction == null);
+ returnToPoolAction = returnAction;
+ }
+
public RequestCallCompletionDelegate CompletionCallback { get; set; }
// Gets data of server_rpc_new completion.
@@ -76,10 +82,15 @@
public void Recycle()
{
- if (ownedByPool != null)
+ if (returnToPoolAction != null)
{
Native.grpcsharp_request_call_context_reset(this);
- ownedByPool.Return(this);
+
+ var origReturnAction = returnToPoolAction;
+ // Not clearing all the references to the pool could prevent garbage collection of the pool object
+ // and thus cause memory leaks.
+ returnToPoolAction = null;
+ origReturnAction(this);
}
else
{
diff --git a/src/php/ext/grpc/call.c b/src/php/ext/grpc/call.c
index c4997f7..ff55c3c 100644
--- a/src/php/ext/grpc/call.c
+++ b/src/php/ext/grpc/call.c
@@ -99,6 +99,7 @@
1 TSRMLS_CC);
efree(str_key);
efree(str_val);
+ PHP_GRPC_FREE_STD_ZVAL(array);
return NULL;
}
php_grpc_add_next_index_stringl(data, str_val,
@@ -127,10 +128,12 @@
HashTable *inner_array_hash;
zval *value;
zval *inner_array;
+ grpc_metadata_array_init(metadata);
+ metadata->count = 0;
+ metadata->metadata = NULL;
if (Z_TYPE_P(array) != IS_ARRAY) {
return false;
}
- grpc_metadata_array_init(metadata);
array_hash = Z_ARRVAL_P(array);
char *key;
@@ -174,6 +177,18 @@
return true;
}
+void grpc_php_metadata_array_destroy_including_entries(
+ grpc_metadata_array* array) {
+ size_t i;
+ if (array->metadata) {
+ for (i = 0; i < array->count; i++) {
+ grpc_slice_unref(array->metadata[i].key);
+ grpc_slice_unref(array->metadata[i].value);
+ }
+ }
+ grpc_metadata_array_destroy(array);
+}
+
/* Wraps a grpc_call struct in a PHP object. Owned indicates whether the
struct should be destroyed at the end of the object's lifecycle */
zval *grpc_php_wrap_call(grpc_call *wrapped, bool owned TSRMLS_DC) {
@@ -502,8 +517,8 @@
}
cleanup:
- grpc_metadata_array_destroy(&metadata);
- grpc_metadata_array_destroy(&trailing_metadata);
+ grpc_php_metadata_array_destroy_including_entries(&metadata);
+ grpc_php_metadata_array_destroy_including_entries(&trailing_metadata);
grpc_metadata_array_destroy(&recv_metadata);
grpc_metadata_array_destroy(&recv_trailing_metadata);
grpc_slice_unref(recv_status_details);
@@ -526,7 +541,9 @@
*/
PHP_METHOD(Call, getPeer) {
wrapped_grpc_call *call = Z_WRAPPED_GRPC_CALL_P(getThis());
- PHP_GRPC_RETURN_STRING(grpc_call_get_peer(call->wrapped), 1);
+ char *peer = grpc_call_get_peer(call->wrapped);
+ PHP_GRPC_RETVAL_STRING(peer, 1);
+ gpr_free(peer);
}
/**
diff --git a/src/php/ext/grpc/call.h b/src/php/ext/grpc/call.h
index 5bde5d5..104ac30 100644
--- a/src/php/ext/grpc/call.h
+++ b/src/php/ext/grpc/call.h
@@ -69,5 +69,6 @@
/* Populates a grpc_metadata_array with the data in a PHP array object.
Returns true on success and false on failure */
bool create_metadata_array(zval *array, grpc_metadata_array *metadata);
-
+void grpc_php_metadata_array_destroy_including_entries(
+ grpc_metadata_array* array);
#endif /* NET_GRPC_PHP_GRPC_CHANNEL_H_ */
diff --git a/src/php/ext/grpc/call_credentials.c b/src/php/ext/grpc/call_credentials.c
index a395d53..41c488a 100644
--- a/src/php/ext/grpc/call_credentials.c
+++ b/src/php/ext/grpc/call_credentials.c
@@ -120,6 +120,8 @@
fci->params, fci->param_count) == FAILURE) {
zend_throw_exception(spl_ce_InvalidArgumentException,
"createFromPlugin expects 1 callback", 1 TSRMLS_CC);
+ free(fci);
+ free(fci_cache);
return;
}
@@ -183,15 +185,17 @@
*status = GRPC_STATUS_OK;
*error_details = NULL;
+ bool should_return = false;
grpc_metadata_array metadata;
if (retval == NULL || Z_TYPE_P(retval) != IS_ARRAY) {
*status = GRPC_STATUS_INVALID_ARGUMENT;
- return true; // Synchronous return.
+ should_return = true; // Synchronous return.
}
if (!create_metadata_array(retval, &metadata)) {
*status = GRPC_STATUS_INVALID_ARGUMENT;
- return true; // Synchronous return.
+ should_return = true; // Synchronous return.
+ grpc_php_metadata_array_destroy_including_entries(&metadata);
}
if (retval != NULL) {
@@ -204,6 +208,9 @@
PHP_GRPC_FREE_STD_ZVAL(retval);
#endif
}
+ if (should_return) {
+ return true;
+ }
if (metadata.count > GRPC_METADATA_CREDENTIALS_PLUGIN_SYNC_MAX) {
*status = GRPC_STATUS_INTERNAL;
diff --git a/src/php/ext/grpc/channel.c b/src/php/ext/grpc/channel.c
index db59869..4054723 100644
--- a/src/php/ext/grpc/channel.c
+++ b/src/php/ext/grpc/channel.c
@@ -41,6 +41,7 @@
#include <grpc/grpc.h>
#include <grpc/grpc_security.h>
+#include <grpc/support/alloc.h>
#include "completion_queue.h"
#include "channel_credentials.h"
@@ -56,22 +57,63 @@
/* Frees and destroys an instance of wrapped_grpc_channel */
PHP_GRPC_FREE_WRAPPED_FUNC_START(wrapped_grpc_channel)
+ bool is_last_wrapper = false;
+ // In_persistent_list is used when the user don't close the channel.
+ // In this case, le in the list won't be freed.
+ bool in_persistent_list = true;
if (p->wrapper != NULL) {
gpr_mu_lock(&p->wrapper->mu);
if (p->wrapper->wrapped != NULL) {
- php_grpc_zend_resource *rsrc;
- php_grpc_int key_len = strlen(p->wrapper->key);
- // only destroy the channel here if not found in the persistent list
- gpr_mu_lock(&global_persistent_list_mu);
- if (!(PHP_GRPC_PERSISTENT_LIST_FIND(&EG(persistent_list), p->wrapper->key,
- key_len, rsrc))) {
- grpc_channel_destroy(p->wrapper->wrapped);
- free(p->wrapper->target);
- free(p->wrapper->args_hashstr);
+ if (p->wrapper->is_valid) {
+ php_grpc_zend_resource *rsrc;
+ php_grpc_int key_len = strlen(p->wrapper->key);
+ // only destroy the channel here if not found in the persistent list
+ gpr_mu_lock(&global_persistent_list_mu);
+ if (!(PHP_GRPC_PERSISTENT_LIST_FIND(&EG(persistent_list), p->wrapper->key,
+ key_len, rsrc))) {
+ in_persistent_list = false;
+ grpc_channel_destroy(p->wrapper->wrapped);
+ free(p->wrapper->target);
+ free(p->wrapper->args_hashstr);
+ if(p->wrapper->creds_hashstr != NULL){
+ free(p->wrapper->creds_hashstr);
+ p->wrapper->creds_hashstr = NULL;
+ }
+ }
+ gpr_mu_unlock(&global_persistent_list_mu);
}
- gpr_mu_unlock(&global_persistent_list_mu);
+ }
+ p->wrapper->ref_count -= 1;
+ if (p->wrapper->ref_count == 0) {
+ is_last_wrapper = true;
}
gpr_mu_unlock(&p->wrapper->mu);
+ if (is_last_wrapper) {
+ if (in_persistent_list) {
+ // If ref_count==0 and the key still in the list, it means the user
+ // don't call channel->close().persistent list should free the
+ // allocation in such case, as well as related wrapped channel.
+ if (p->wrapper->wrapped != NULL) {
+ gpr_mu_lock(&p->wrapper->mu);
+ grpc_channel_destroy(p->wrapper->wrapped);
+ free(p->wrapper->target);
+ free(p->wrapper->args_hashstr);
+ if(p->wrapper->creds_hashstr != NULL){
+ free(p->wrapper->creds_hashstr);
+ p->wrapper->creds_hashstr = NULL;
+ }
+ p->wrapper->wrapped = NULL;
+ php_grpc_delete_persistent_list_entry(p->wrapper->key,
+ strlen(p->wrapper->key)
+ TSRMLS_CC);
+ gpr_mu_unlock(&p->wrapper->mu);
+ }
+ }
+ gpr_mu_destroy(&p->wrapper->mu);
+ free(p->wrapper->key);
+ free(p->wrapper);
+ }
+ p->wrapper = NULL;
}
PHP_GRPC_FREE_WRAPPED_FUNC_END()
@@ -276,9 +318,16 @@
channel->wrapper->key = key;
channel->wrapper->target = strdup(target);
channel->wrapper->args_hashstr = strdup(sha1str);
+ channel->wrapper->creds_hashstr = NULL;
+ channel->wrapper->ref_count = 1;
+ channel->wrapper->is_valid = true;
if (creds != NULL && creds->hashstr != NULL) {
- channel->wrapper->creds_hashstr = creds->hashstr;
+ php_grpc_int creds_hashstr_len = strlen(creds->hashstr);
+ char *channel_creds_hashstr = malloc(creds_hashstr_len + 1);
+ strcpy(channel_creds_hashstr, creds->hashstr);
+ channel->wrapper->creds_hashstr = channel_creds_hashstr;
}
+
gpr_mu_init(&channel->wrapper->mu);
smart_str_free(&buf);
@@ -303,7 +352,17 @@
channel, target, args, creds, key, key_len TSRMLS_CC);
} else {
efree(args.args);
+ if (channel->wrapper->creds_hashstr != NULL){
+ free(channel->wrapper->creds_hashstr);
+ channel->wrapper->creds_hashstr = NULL;
+ }
+ free(channel->wrapper->creds_hashstr);
+ free(channel->wrapper->key);
+ free(channel->wrapper->target);
+ free(channel->wrapper->args_hashstr);
+ free(channel->wrapper);
channel->wrapper = le->channel;
+ channel->wrapper->ref_count += 1;
}
}
}
@@ -323,7 +382,8 @@
}
char *target = grpc_channel_get_target(channel->wrapper->wrapped);
gpr_mu_unlock(&channel->wrapper->mu);
- PHP_GRPC_RETURN_STRING(target, 1);
+ PHP_GRPC_RETVAL_STRING(target, 1);
+ gpr_free(target);
}
/**
@@ -411,18 +471,46 @@
*/
PHP_METHOD(Channel, close) {
wrapped_grpc_channel *channel = Z_WRAPPED_GRPC_CHANNEL_P(getThis());
- gpr_mu_lock(&channel->wrapper->mu);
- if (channel->wrapper->wrapped != NULL) {
- grpc_channel_destroy(channel->wrapper->wrapped);
- free(channel->wrapper->target);
- free(channel->wrapper->args_hashstr);
- channel->wrapper->wrapped = NULL;
+ bool is_last_wrapper = false;
+ if (channel->wrapper != NULL) {
+ // Channel_wrapper hasn't call close before.
+ gpr_mu_lock(&channel->wrapper->mu);
+ if (channel->wrapper->wrapped != NULL) {
+ if (channel->wrapper->is_valid) {
+ // Wrapped channel hasn't been destoryed by other wrapper.
+ grpc_channel_destroy(channel->wrapper->wrapped);
+ free(channel->wrapper->target);
+ free(channel->wrapper->args_hashstr);
+ if(channel->wrapper->creds_hashstr != NULL){
+ free(channel->wrapper->creds_hashstr);
+ channel->wrapper->creds_hashstr = NULL;
+ }
+ channel->wrapper->wrapped = NULL;
+ channel->wrapper->is_valid = false;
- php_grpc_delete_persistent_list_entry(channel->wrapper->key,
- strlen(channel->wrapper->key)
- TSRMLS_CC);
+ php_grpc_delete_persistent_list_entry(channel->wrapper->key,
+ strlen(channel->wrapper->key)
+ TSRMLS_CC);
+ }
+ }
+ channel->wrapper->ref_count -= 1;
+ if(channel->wrapper->ref_count == 0){
+ // Mark that the wrapper can be freed because mu should be
+ // destroyed outside the lock.
+ is_last_wrapper = true;
+ }
+ gpr_mu_unlock(&channel->wrapper->mu);
}
- gpr_mu_unlock(&channel->wrapper->mu);
+ gpr_mu_lock(&global_persistent_list_mu);
+ if (is_last_wrapper) {
+ gpr_mu_destroy(&channel->wrapper->mu);
+ free(channel->wrapper->key);
+ free(channel->wrapper);
+ }
+ // Set channel->wrapper to NULL to avoid call close twice for the same
+ // channel.
+ channel->wrapper = NULL;
+ gpr_mu_unlock(&global_persistent_list_mu);
}
// Delete an entry from the persistent list
@@ -437,6 +525,7 @@
le = (channel_persistent_le_t *)rsrc->ptr;
le->channel = NULL;
php_grpc_zend_hash_del(&EG(persistent_list), key, key_len+1);
+ free(le);
}
gpr_mu_unlock(&global_persistent_list_mu);
}
diff --git a/src/php/ext/grpc/channel.h b/src/php/ext/grpc/channel.h
index 69adc47..86bfdea 100755
--- a/src/php/ext/grpc/channel.h
+++ b/src/php/ext/grpc/channel.h
@@ -40,6 +40,11 @@
char *args_hashstr;
char *creds_hashstr;
gpr_mu mu;
+ // is_valid is used to check the wrapped channel has been freed
+ // before to avoid double free.
+ bool is_valid;
+ // ref_count is used to let the last wrapper free related channel and key.
+ size_t ref_count;
} grpc_channel_wrapper;
/* Wrapper struct for grpc_channel that can be associated with a PHP object */
diff --git a/src/php/ext/grpc/channel_credentials.c b/src/php/ext/grpc/channel_credentials.c
index d120d6e..624d7cc 100644
--- a/src/php/ext/grpc/channel_credentials.c
+++ b/src/php/ext/grpc/channel_credentials.c
@@ -57,8 +57,13 @@
/* Frees and destroys an instance of wrapped_grpc_channel_credentials */
PHP_GRPC_FREE_WRAPPED_FUNC_START(wrapped_grpc_channel_credentials)
+ if (p->hashstr != NULL) {
+ free(p->hashstr);
+ p->hashstr = NULL;
+ }
if (p->wrapped != NULL) {
grpc_channel_credentials_release(p->wrapped);
+ p->wrapped = NULL;
}
PHP_GRPC_FREE_WRAPPED_FUNC_END()
@@ -152,7 +157,7 @@
}
php_grpc_int hashkey_len = root_certs_length + cert_chain_length;
- char *hashkey = emalloc(hashkey_len);
+ char *hashkey = emalloc(hashkey_len + 1);
if (root_certs_length > 0) {
strcpy(hashkey, pem_root_certs);
}
@@ -199,8 +204,13 @@
grpc_channel_credentials *creds =
grpc_composite_channel_credentials_create(cred1->wrapped, cred2->wrapped,
NULL);
+ // wrapped_grpc_channel_credentials object should keeps it's own
+ // allocation. Otherwise it conflicts free hashstr with call.c.
+ php_grpc_int cred1_len = strlen(cred1->hashstr);
+ char *cred1_hashstr = malloc(cred1_len+1);
+ strcpy(cred1_hashstr, cred1->hashstr);
zval *creds_object =
- grpc_php_wrap_channel_credentials(creds, cred1->hashstr, true
+ grpc_php_wrap_channel_credentials(creds, cred1_hashstr, true
TSRMLS_CC);
RETURN_DESTROY_ZVAL(creds_object);
}
diff --git a/src/php/ext/grpc/php7_wrapper.h b/src/php/ext/grpc/php7_wrapper.h
index 96091f9..2f4a536 100644
--- a/src/php/ext/grpc/php7_wrapper.h
+++ b/src/php/ext/grpc/php7_wrapper.h
@@ -33,6 +33,7 @@
#define php_grpc_add_next_index_stringl(data, str, len, b) \
add_next_index_stringl(data, str, len, b)
+#define PHP_GRPC_RETVAL_STRING(val, dup) RETVAL_STRING(val, dup)
#define PHP_GRPC_RETURN_STRING(val, dup) RETURN_STRING(val, dup)
#define PHP_GRPC_MAKE_STD_ZVAL(pzv) MAKE_STD_ZVAL(pzv)
#define PHP_GRPC_FREE_STD_ZVAL(pzv)
@@ -145,6 +146,7 @@
#define php_grpc_add_next_index_stringl(data, str, len, b) \
add_next_index_stringl(data, str, len)
+#define PHP_GRPC_RETVAL_STRING(val, dup) RETVAL_STRING(val)
#define PHP_GRPC_RETURN_STRING(val, dup) RETURN_STRING(val)
#define PHP_GRPC_MAKE_STD_ZVAL(pzv) \
pzv = (zval *)emalloc(sizeof(zval));
diff --git a/src/php/ext/grpc/php_grpc.c b/src/php/ext/grpc/php_grpc.c
index 0f2c5b8..5971bab 100644
--- a/src/php/ext/grpc/php_grpc.c
+++ b/src/php/ext/grpc/php_grpc.c
@@ -253,7 +253,8 @@
*/
PHP_MINFO_FUNCTION(grpc) {
php_info_print_table_start();
- php_info_print_table_header(2, "grpc support", "enabled");
+ php_info_print_table_row(2, "grpc support", "enabled");
+ php_info_print_table_row(2, "grpc module version", PHP_GRPC_VERSION);
php_info_print_table_end();
/* Remove comments if you have entries in php.ini
diff --git a/templates/package.xml.template b/templates/package.xml.template
index f10f75b..1232179 100644
--- a/templates/package.xml.template
+++ b/templates/package.xml.template
@@ -12,24 +12,22 @@
<email>grpc-packages@google.com</email>
<active>yes</active>
</lead>
- <date>2017-08-24</date>
+ <date>2018-01-23</date>
<time>16:06:07</time>
<version>
<release>${settings.php_version.php()}</release>
<api>${settings.php_version.php()}</api>
</version>
<stability>
- <release>beta</release>
- <api>beta</api>
+ <release>${settings.php_version.php_stability()}</release>
+ <api>${settings.php_version.php_stability()}</api>
</stability>
<license>Apache 2.0</license>
<notes>
- - Channel are now by default persistent #11878
- - Some bug fixes from 1.4 branch #12109, #12123
- - Fixed hang bug when fork() was used #11814
- - License changed to Apache 2.0
- - Added support for php_namespace option in codegen plugin #11886
- - Updated gRPC C Core library version 1.6
+ - Updated gRPC C Core library version 1.9
+ - Report grpc extension version in phpinfo() #13687
+ - Fixed memory leak when handling metadata array #13660
+ - Fixed memory involing persistent channels #14125 - #14130
</notes>
<contents>
<dir baseinstalldir="/" name="/">
diff --git a/tools/buildgen/plugins/expand_version.py b/tools/buildgen/plugins/expand_version.py
index 8f56ce8..facf349 100755
--- a/tools/buildgen/plugins/expand_version.py
+++ b/tools/buildgen/plugins/expand_version.py
@@ -84,6 +84,13 @@
% self.tag)
return s
+ def php_stability(self):
+ """stability string for PHP PECL package.xml file"""
+ if self.tag:
+ return 'beta'
+ else:
+ return 'stable'
+
def php_composer(self):
"""Version string for PHP Composer package"""
return '%d.%d.%d' % (self.major, self.minor, self.patch)
