From 2ddb5a6cb54c88ccf36af81f79160150a40df0be Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 8 Jun 2015 17:51:36 -0700 Subject: [PATCH 1/6] revamp of c# channel options --- .../Grpc.Core.Tests/ChannelOptionsTest.cs | 105 ++++++++++ .../Grpc.Core.Tests/Grpc.Core.Tests.csproj | 6 +- .../Internal/ChannelArgsSafeHandleTest.cs | 75 ++++++++ src/csharp/Grpc.Core/Channel.cs | 40 ++-- src/csharp/Grpc.Core/ChannelArgs.cs | 115 ----------- src/csharp/Grpc.Core/ChannelOptions.cs | 179 ++++++++++++++++++ src/csharp/Grpc.Core/Grpc.Core.csproj | 6 +- .../Internal/ChannelArgsSafeHandle.cs | 8 + .../Grpc.IntegrationTesting/InteropClient.cs | 9 +- .../InteropClientServerTest.cs | 8 +- src/csharp/ext/grpc_csharp_ext.c | 10 + 11 files changed, 413 insertions(+), 148 deletions(-) create mode 100644 src/csharp/Grpc.Core.Tests/ChannelOptionsTest.cs create mode 100644 src/csharp/Grpc.Core.Tests/Internal/ChannelArgsSafeHandleTest.cs delete mode 100644 src/csharp/Grpc.Core/ChannelArgs.cs create mode 100644 src/csharp/Grpc.Core/ChannelOptions.cs diff --git a/src/csharp/Grpc.Core.Tests/ChannelOptionsTest.cs b/src/csharp/Grpc.Core.Tests/ChannelOptionsTest.cs new file mode 100644 index 00000000000..ee846eae96b --- /dev/null +++ b/src/csharp/Grpc.Core.Tests/ChannelOptionsTest.cs @@ -0,0 +1,105 @@ +#region Copyright notice and license + +// Copyright 2015, Google Inc. +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +#endregion + +using System; +using System.Collections.Generic; +using Grpc.Core; +using Grpc.Core.Internal; +using Grpc.Core.Utils; +using NUnit.Framework; + +namespace Grpc.Core.Internal.Tests +{ + public class ChannelOptionsTest + { + [Test] + public void IntOption() + { + var option = new ChannelOption("somename", 1); + + Assert.AreEqual(ChannelOption.OptionType.Integer, option.Type); + Assert.AreEqual("somename", option.Name); + Assert.AreEqual(1, option.IntValue); + Assert.Throws(typeof(InvalidOperationException), () => {var s = option.StringValue; }); + } + + [Test] + public void StringOption() + { + var option = new ChannelOption("somename", "ABCDEF"); + + Assert.AreEqual(ChannelOption.OptionType.String, option.Type); + Assert.AreEqual("somename", option.Name); + Assert.AreEqual("ABCDEF", option.StringValue); + Assert.Throws(typeof(InvalidOperationException), () => {var s = option.IntValue; }); + } + + [Test] + public void ConstructorPreconditions() + { + Assert.Throws(typeof(NullReferenceException), () => { new ChannelOption(null, "abc"); }); + Assert.Throws(typeof(NullReferenceException), () => { new ChannelOption(null, 1); }); + Assert.Throws(typeof(NullReferenceException), () => { new ChannelOption("abc", null); }); + } + + [Test] + public void CreateChannelArgsNull() + { + var channelArgs = ChannelOptions.CreateChannelArgs(null); + Assert.IsTrue(channelArgs.IsInvalid); + } + + [Test] + public void CreateChannelArgsEmpty() + { + var options = new List(); + var channelArgs = ChannelOptions.CreateChannelArgs(options); + channelArgs.Dispose(); + } + + [Test] + public void CreateChannelArgs() + { + var options = new List + { + new ChannelOption("ABC", "XYZ"), + new ChannelOption("somename", "IJKLM"), + new ChannelOption("intoption", 12345), + new ChannelOption("GHIJK", 12345), + }; + + var channelArgs = ChannelOptions.CreateChannelArgs(options); + channelArgs.Dispose(); + } + } +} diff --git a/src/csharp/Grpc.Core.Tests/Grpc.Core.Tests.csproj b/src/csharp/Grpc.Core.Tests/Grpc.Core.Tests.csproj index 029653967b6..92e28b7d74c 100644 --- a/src/csharp/Grpc.Core.Tests/Grpc.Core.Tests.csproj +++ b/src/csharp/Grpc.Core.Tests/Grpc.Core.Tests.csproj @@ -3,8 +3,6 @@ Debug AnyCPU - 8.0.30703 - 2.0 {86EC5CB4-4EA2-40A2-8057-86542A0353BB} Library Grpc.Core.Tests @@ -48,6 +46,8 @@ + + @@ -63,4 +63,4 @@ - \ No newline at end of file + diff --git a/src/csharp/Grpc.Core.Tests/Internal/ChannelArgsSafeHandleTest.cs b/src/csharp/Grpc.Core.Tests/Internal/ChannelArgsSafeHandleTest.cs new file mode 100644 index 00000000000..af0aaa5f012 --- /dev/null +++ b/src/csharp/Grpc.Core.Tests/Internal/ChannelArgsSafeHandleTest.cs @@ -0,0 +1,75 @@ +#region Copyright notice and license + +// Copyright 2015, Google Inc. +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +#endregion + +using System; +using Grpc.Core; +using Grpc.Core.Internal; +using Grpc.Core.Utils; +using NUnit.Framework; + +namespace Grpc.Core.Internal.Tests +{ + public class ChannelArgsSafeHandleTest + { + [Test] + public void CreateEmptyAndDestroy() + { + var channelArgs = ChannelArgsSafeHandle.Create(0); + channelArgs.Dispose(); + } + + [Test] + public void CreateNonEmptyAndDestroy() + { + var channelArgs = ChannelArgsSafeHandle.Create(5); + channelArgs.Dispose(); + } + + [Test] + public void CreateNullAndDestroy() + { + var channelArgs = ChannelArgsSafeHandle.CreateNull(); + channelArgs.Dispose(); + } + + [Test] + public void CreateFillAndDestroy() + { + var channelArgs = ChannelArgsSafeHandle.Create(3); + channelArgs.SetInteger(0, "somekey", 12345); + channelArgs.SetString(1, "somekey", "abcdefghijkl"); + channelArgs.SetString(2, "somekey", "XYZ"); + channelArgs.Dispose(); + } + } +} diff --git a/src/csharp/Grpc.Core/Channel.cs b/src/csharp/Grpc.Core/Channel.cs index 44b610f65b8..d6bfbb7bc45 100644 --- a/src/csharp/Grpc.Core/Channel.cs +++ b/src/csharp/Grpc.Core/Channel.cs @@ -29,6 +29,7 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #endregion using System; +using System.Collections.Generic; using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; @@ -50,10 +51,10 @@ namespace Grpc.Core /// /// The DNS name of IP address of the host. /// Optional credentials to create a secure channel. - /// Optional channel arguments. - public Channel(string host, Credentials credentials = null, ChannelArgs channelArgs = null) + /// Channel options. + public Channel(string host, Credentials credentials = null, IEnumerable options = null) { - using (ChannelArgsSafeHandle nativeChannelArgs = CreateNativeChannelArgs(channelArgs)) + using (ChannelArgsSafeHandle nativeChannelArgs = ChannelOptions.CreateChannelArgs(options)) { if (credentials != null) { @@ -67,7 +68,7 @@ namespace Grpc.Core this.handle = ChannelSafeHandle.Create(host, nativeChannelArgs); } } - this.target = GetOverridenTarget(host, channelArgs); + this.target = GetOverridenTarget(host, options); } /// @@ -76,9 +77,9 @@ namespace Grpc.Core /// DNS name or IP address /// the port /// Optional credentials to create a secure channel. - /// Optional channel arguments. - public Channel(string host, int port, Credentials credentials = null, ChannelArgs channelArgs = null) : - this(string.Format("{0}:{1}", host, port), credentials, channelArgs) + /// Channel options. + public Channel(string host, int port, Credentials credentials = null, IEnumerable options = null) : + this(string.Format("{0}:{1}", host, port), credentials, options) { } @@ -112,22 +113,25 @@ namespace Grpc.Core } } - private static string GetOverridenTarget(string target, ChannelArgs args) + /// + /// Look for SslTargetNameOverride option and return its value instead of originalTarget + /// if found. + /// + private static string GetOverridenTarget(string originalTarget, IEnumerable options) { - if (args != null && !string.IsNullOrEmpty(args.GetSslTargetNameOverride())) + if (options == null) { - return args.GetSslTargetNameOverride(); + return originalTarget; } - return target; - } - - private static ChannelArgsSafeHandle CreateNativeChannelArgs(ChannelArgs args) - { - if (args == null) + foreach (var option in options) { - return ChannelArgsSafeHandle.CreateNull(); + if (option.Type == ChannelOption.OptionType.String + && option.Name == ChannelOptions.SslTargetNameOverride) + { + return option.StringValue; + } } - return args.ToNativeChannelArgs(); + return originalTarget; } } } diff --git a/src/csharp/Grpc.Core/ChannelArgs.cs b/src/csharp/Grpc.Core/ChannelArgs.cs deleted file mode 100644 index 74ab310e44e..00000000000 --- a/src/csharp/Grpc.Core/ChannelArgs.cs +++ /dev/null @@ -1,115 +0,0 @@ -#region Copyright notice and license -// Copyright 2015, Google Inc. -// All rights reserved. -// -// Redistribution and use in source and binary forms, with or without -// modification, are permitted provided that the following conditions are -// met: -// -// * Redistributions of source code must retain the above copyright -// notice, this list of conditions and the following disclaimer. -// * Redistributions in binary form must reproduce the above -// copyright notice, this list of conditions and the following disclaimer -// in the documentation and/or other materials provided with the -// distribution. -// * Neither the name of Google Inc. nor the names of its -// contributors may be used to endorse or promote products derived from -// this software without specific prior written permission. -// -// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -#endregion -using System; -using System.Collections.Generic; -using System.Collections.Immutable; -using System.Runtime.InteropServices; -using System.Threading; -using System.Threading.Tasks; -using Grpc.Core.Internal; - -namespace Grpc.Core -{ - /// - /// gRPC channel options. - /// - public class ChannelArgs - { - public const string SslTargetNameOverrideKey = "grpc.ssl_target_name_override"; - - readonly ImmutableDictionary stringArgs; - - private ChannelArgs(ImmutableDictionary stringArgs) - { - this.stringArgs = stringArgs; - } - - public string GetSslTargetNameOverride() - { - string result; - if (stringArgs.TryGetValue(SslTargetNameOverrideKey, out result)) - { - return result; - } - return null; - } - - public static Builder CreateBuilder() - { - return new Builder(); - } - - public class Builder - { - readonly Dictionary stringArgs = new Dictionary(); - - // TODO: AddInteger not supported yet. - public Builder AddString(string key, string value) - { - stringArgs.Add(key, value); - return this; - } - - public ChannelArgs Build() - { - return new ChannelArgs(stringArgs.ToImmutableDictionary()); - } - } - - /// - /// Creates native object for the channel arguments. - /// - /// The native channel arguments. - internal ChannelArgsSafeHandle ToNativeChannelArgs() - { - ChannelArgsSafeHandle nativeArgs = null; - try - { - nativeArgs = ChannelArgsSafeHandle.Create(stringArgs.Count); - int i = 0; - foreach (var entry in stringArgs) - { - nativeArgs.SetString(i, entry.Key, entry.Value); - i++; - } - return nativeArgs; - } - catch (Exception) - { - if (nativeArgs != null) - { - nativeArgs.Dispose(); - } - throw; - } - } - } -} diff --git a/src/csharp/Grpc.Core/ChannelOptions.cs b/src/csharp/Grpc.Core/ChannelOptions.cs new file mode 100644 index 00000000000..4d37b396820 --- /dev/null +++ b/src/csharp/Grpc.Core/ChannelOptions.cs @@ -0,0 +1,179 @@ +#region Copyright notice and license +// Copyright 2015, Google Inc. +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +#endregion +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Runtime.InteropServices; +using System.Threading; +using System.Threading.Tasks; +using Grpc.Core.Internal; +using Grpc.Core.Utils; + +namespace Grpc.Core +{ + /// + /// Channel option specified when creating a channel. + /// Corresponds to grpc_channel_args from grpc/grpc.h. + /// + public sealed class ChannelOption + { + public enum OptionType + { + Integer, + String + } + + private readonly OptionType type; + private readonly string name; + private readonly int intValue; + private readonly string stringValue; + + /// + /// Creates a channel option with a string value. + /// + /// Name. + /// String value. + public ChannelOption(string name, string stringValue) + { + this.type = OptionType.String; + this.name = Preconditions.CheckNotNull(name); + this.stringValue = Preconditions.CheckNotNull(stringValue); + } + + /// + /// Creates a channel option with an integer value. + /// + /// Name. + /// String value. + public ChannelOption(string name, int intValue) + { + this.type = OptionType.Integer; + this.name = Preconditions.CheckNotNull(name); + this.intValue = intValue; + } + + public OptionType Type + { + get + { + return type; + } + } + + public string Name + { + get + { + return name; + } + } + + public int IntValue + { + get + { + Preconditions.CheckState(type == OptionType.Integer); + return intValue; + } + } + + public string StringValue + { + get + { + Preconditions.CheckState(type == OptionType.String); + return stringValue; + } + } + } + + public static class ChannelOptions + { + // Override SSL target check. Only to be used for testing. + public const string SslTargetNameOverride = "grpc.ssl_target_name_override"; + + // Enable census for tracing and stats collection + public const string Census = "grpc.census"; + + // Maximum number of concurrent incoming streams to allow on a http2 connection + public const string MaxConcurrentStreams = "grpc.max_concurrent_streams"; + + // Maximum message length that the channel can receive + public const string MaxMessageLength = "grpc.max_message_length"; + + // Initial sequence number for http2 transports + public const string Http2InitialSequenceNumber = "grpc.http2.initial_sequence_number"; + + /// + /// Creates native object for a collection of channel options. + /// + /// The native channel arguments. + internal static ChannelArgsSafeHandle CreateChannelArgs(IEnumerable options) + { + if (options == null) + { + return ChannelArgsSafeHandle.CreateNull(); + } + var optionList = new List(options); // It's better to do defensive copy + ChannelArgsSafeHandle nativeArgs = null; + try + { + nativeArgs = ChannelArgsSafeHandle.Create(optionList.Count); + for (int i = 0; i < optionList.Count; i++) + { + var option = optionList[i]; + if (option.Type == ChannelOption.OptionType.Integer) + { + nativeArgs.SetInteger(i, option.Name, option.IntValue); + } + else if (option.Type == ChannelOption.OptionType.String) + { + nativeArgs.SetString(i, option.Name, option.StringValue); + } + else + { + throw new InvalidOperationException("Unknown option type"); + } + + } + return nativeArgs; + } + catch (Exception) + { + if (nativeArgs != null) + { + nativeArgs.Dispose(); + } + throw; + } + } + } +} diff --git a/src/csharp/Grpc.Core/Grpc.Core.csproj b/src/csharp/Grpc.Core/Grpc.Core.csproj index 5c7b9a8bb67..a36a6a5acc8 100644 --- a/src/csharp/Grpc.Core/Grpc.Core.csproj +++ b/src/csharp/Grpc.Core/Grpc.Core.csproj @@ -5,8 +5,6 @@ Debug AnyCPU - 8.0.30703 - 2.0 {CCC4440E-49F7-4790-B0AF-FEABB0837AE7} Library Grpc.Core @@ -78,7 +76,6 @@ - @@ -103,6 +100,7 @@ + @@ -132,4 +130,4 @@ - \ No newline at end of file + diff --git a/src/csharp/Grpc.Core/Internal/ChannelArgsSafeHandle.cs b/src/csharp/Grpc.Core/Internal/ChannelArgsSafeHandle.cs index c69f1a0d025..c12aec5a3a4 100644 --- a/src/csharp/Grpc.Core/Internal/ChannelArgsSafeHandle.cs +++ b/src/csharp/Grpc.Core/Internal/ChannelArgsSafeHandle.cs @@ -45,6 +45,9 @@ namespace Grpc.Core.Internal [DllImport("grpc_csharp_ext.dll", CharSet = CharSet.Ansi)] static extern void grpcsharp_channel_args_set_string(ChannelArgsSafeHandle args, UIntPtr index, string key, string value); + [DllImport("grpc_csharp_ext.dll", CharSet = CharSet.Ansi)] + static extern void grpcsharp_channel_args_set_integer(ChannelArgsSafeHandle args, UIntPtr index, string key, int value); + [DllImport("grpc_csharp_ext.dll")] static extern void grpcsharp_channel_args_destroy(IntPtr args); @@ -67,6 +70,11 @@ namespace Grpc.Core.Internal grpcsharp_channel_args_set_string(this, new UIntPtr((uint)index), key, value); } + public void SetInteger(int index, string key, int value) + { + grpcsharp_channel_args_set_integer(this, new UIntPtr((uint)index), key, value); + } + protected override bool ReleaseHandle() { grpcsharp_channel_args_destroy(handle); diff --git a/src/csharp/Grpc.IntegrationTesting/InteropClient.cs b/src/csharp/Grpc.IntegrationTesting/InteropClient.cs index 66171fae576..faee5a8fa56 100644 --- a/src/csharp/Grpc.IntegrationTesting/InteropClient.cs +++ b/src/csharp/Grpc.IntegrationTesting/InteropClient.cs @@ -110,14 +110,15 @@ namespace Grpc.IntegrationTesting credentials = TestCredentials.CreateTestClientCredentials(options.useTestCa); } - ChannelArgs channelArgs = null; + List channelOptions = null; if (!string.IsNullOrEmpty(options.serverHostOverride)) { - channelArgs = ChannelArgs.CreateBuilder() - .AddString(ChannelArgs.SslTargetNameOverrideKey, options.serverHostOverride).Build(); + channelOptions = new List { + new ChannelOption(ChannelOptions.SslTargetNameOverride, options.serverHostOverride) + }; } - using (Channel channel = new Channel(options.serverHost, options.serverPort.Value, credentials, channelArgs)) + using (Channel channel = new Channel(options.serverHost, options.serverPort.Value, credentials, channelOptions)) { var stubConfig = StubConfiguration.Default; if (options.testCase == "service_account_creds" || options.testCase == "compute_engine_creds") diff --git a/src/csharp/Grpc.IntegrationTesting/InteropClientServerTest.cs b/src/csharp/Grpc.IntegrationTesting/InteropClientServerTest.cs index f756dfbc409..33628ce4c76 100644 --- a/src/csharp/Grpc.IntegrationTesting/InteropClientServerTest.cs +++ b/src/csharp/Grpc.IntegrationTesting/InteropClientServerTest.cs @@ -62,10 +62,10 @@ namespace Grpc.IntegrationTesting int port = server.AddListeningPort(host, Server.PickUnusedPort, TestCredentials.CreateTestServerCredentials()); server.Start(); - var channelArgs = ChannelArgs.CreateBuilder() - .AddString(ChannelArgs.SslTargetNameOverrideKey, TestCredentials.DefaultHostOverride).Build(); - - channel = new Channel(host, port, TestCredentials.CreateTestClientCredentials(true), channelArgs); + var options = new List { + new ChannelOption(ChannelOptions.SslTargetNameOverride, TestCredentials.DefaultHostOverride) + }; + channel = new Channel(host, port, TestCredentials.CreateTestClientCredentials(true), options); client = TestService.NewStub(channel); } diff --git a/src/csharp/ext/grpc_csharp_ext.c b/src/csharp/ext/grpc_csharp_ext.c index 57be445331a..59b8993ad3d 100644 --- a/src/csharp/ext/grpc_csharp_ext.c +++ b/src/csharp/ext/grpc_csharp_ext.c @@ -354,6 +354,16 @@ grpcsharp_channel_args_set_string(grpc_channel_args *args, size_t index, args->args[index].value.string = gpr_strdup(value); } +GPR_EXPORT void GPR_CALLTYPE +grpcsharp_channel_args_set_integer(grpc_channel_args *args, size_t index, + const char *key, int value) { + GPR_ASSERT(args); + GPR_ASSERT(index < args->num_args); + args->args[index].type = GRPC_ARG_INTEGER; + args->args[index].key = gpr_strdup(key); + args->args[index].value.integer = value; +} + GPR_EXPORT void GPR_CALLTYPE grpcsharp_channel_args_destroy(grpc_channel_args *args) { size_t i; From c8f7d1079e66a87e1248462b5ff2216460f488ab Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 8 Jun 2015 17:53:45 -0700 Subject: [PATCH 2/6] fix stylecop issues --- src/csharp/Grpc.Core.Tests/ChannelOptionsTest.cs | 4 ++-- src/csharp/Grpc.Core/ChannelOptions.cs | 1 - src/csharp/Grpc.IntegrationTesting/InteropClient.cs | 3 ++- src/csharp/Grpc.IntegrationTesting/InteropClientServerTest.cs | 3 ++- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/csharp/Grpc.Core.Tests/ChannelOptionsTest.cs b/src/csharp/Grpc.Core.Tests/ChannelOptionsTest.cs index ee846eae96b..df09857efe3 100644 --- a/src/csharp/Grpc.Core.Tests/ChannelOptionsTest.cs +++ b/src/csharp/Grpc.Core.Tests/ChannelOptionsTest.cs @@ -50,7 +50,7 @@ namespace Grpc.Core.Internal.Tests Assert.AreEqual(ChannelOption.OptionType.Integer, option.Type); Assert.AreEqual("somename", option.Name); Assert.AreEqual(1, option.IntValue); - Assert.Throws(typeof(InvalidOperationException), () => {var s = option.StringValue; }); + Assert.Throws(typeof(InvalidOperationException), () => { var s = option.StringValue; }); } [Test] @@ -61,7 +61,7 @@ namespace Grpc.Core.Internal.Tests Assert.AreEqual(ChannelOption.OptionType.String, option.Type); Assert.AreEqual("somename", option.Name); Assert.AreEqual("ABCDEF", option.StringValue); - Assert.Throws(typeof(InvalidOperationException), () => {var s = option.IntValue; }); + Assert.Throws(typeof(InvalidOperationException), () => { var s = option.IntValue; }); } [Test] diff --git a/src/csharp/Grpc.Core/ChannelOptions.cs b/src/csharp/Grpc.Core/ChannelOptions.cs index 4d37b396820..bc23bb59b10 100644 --- a/src/csharp/Grpc.Core/ChannelOptions.cs +++ b/src/csharp/Grpc.Core/ChannelOptions.cs @@ -162,7 +162,6 @@ namespace Grpc.Core { throw new InvalidOperationException("Unknown option type"); } - } return nativeArgs; } diff --git a/src/csharp/Grpc.IntegrationTesting/InteropClient.cs b/src/csharp/Grpc.IntegrationTesting/InteropClient.cs index faee5a8fa56..f0be522bc6e 100644 --- a/src/csharp/Grpc.IntegrationTesting/InteropClient.cs +++ b/src/csharp/Grpc.IntegrationTesting/InteropClient.cs @@ -113,7 +113,8 @@ namespace Grpc.IntegrationTesting List channelOptions = null; if (!string.IsNullOrEmpty(options.serverHostOverride)) { - channelOptions = new List { + channelOptions = new List + { new ChannelOption(ChannelOptions.SslTargetNameOverride, options.serverHostOverride) }; } diff --git a/src/csharp/Grpc.IntegrationTesting/InteropClientServerTest.cs b/src/csharp/Grpc.IntegrationTesting/InteropClientServerTest.cs index 33628ce4c76..1a733450c1a 100644 --- a/src/csharp/Grpc.IntegrationTesting/InteropClientServerTest.cs +++ b/src/csharp/Grpc.IntegrationTesting/InteropClientServerTest.cs @@ -62,7 +62,8 @@ namespace Grpc.IntegrationTesting int port = server.AddListeningPort(host, Server.PickUnusedPort, TestCredentials.CreateTestServerCredentials()); server.Start(); - var options = new List { + var options = new List + { new ChannelOption(ChannelOptions.SslTargetNameOverride, TestCredentials.DefaultHostOverride) }; channel = new Channel(host, port, TestCredentials.CreateTestClientCredentials(true), options); From 6d53a5c60b63f38334140e1e7de9c448a030bc4d Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 8 Jun 2015 18:03:05 -0700 Subject: [PATCH 3/6] allow specifying channel options when creating a server --- .../Grpc.Core/Internal/ServerSafeHandle.cs | 4 ++-- src/csharp/Grpc.Core/Server.cs | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/csharp/Grpc.Core/Internal/ServerSafeHandle.cs b/src/csharp/Grpc.Core/Internal/ServerSafeHandle.cs index edd9c490ffc..9fda1f65691 100644 --- a/src/csharp/Grpc.Core/Internal/ServerSafeHandle.cs +++ b/src/csharp/Grpc.Core/Internal/ServerSafeHandle.cs @@ -45,7 +45,7 @@ namespace Grpc.Core.Internal internal sealed class ServerSafeHandle : SafeHandleZeroIsInvalid { [DllImport("grpc_csharp_ext.dll")] - static extern ServerSafeHandle grpcsharp_server_create(CompletionQueueSafeHandle cq, IntPtr args); + static extern ServerSafeHandle grpcsharp_server_create(CompletionQueueSafeHandle cq, ChannelArgsSafeHandle args); [DllImport("grpc_csharp_ext.dll")] static extern int grpcsharp_server_add_http2_port(ServerSafeHandle server, string addr); @@ -72,7 +72,7 @@ namespace Grpc.Core.Internal { } - public static ServerSafeHandle NewServer(CompletionQueueSafeHandle cq, IntPtr args) + public static ServerSafeHandle NewServer(CompletionQueueSafeHandle cq, ChannelArgsSafeHandle args) { return grpcsharp_server_create(cq, args); } diff --git a/src/csharp/Grpc.Core/Server.cs b/src/csharp/Grpc.Core/Server.cs index da59fc72328..de10be39abf 100644 --- a/src/csharp/Grpc.Core/Server.cs +++ b/src/csharp/Grpc.Core/Server.cs @@ -52,9 +52,6 @@ namespace Grpc.Core /// public const int PickUnusedPort = 0; - //readonly OpCompletionDelegate serverShutdownHandler; - //readonly OpCompletionDelegate newServerRpcHandler; - readonly ServerSafeHandle handle; readonly object myLock = new object(); @@ -64,11 +61,16 @@ namespace Grpc.Core bool startRequested; bool shutdownRequested; - public Server() + /// + /// Create a new server. + /// + /// Channel options. + public Server(IEnumerable options = null) { - this.handle = ServerSafeHandle.NewServer(GetCompletionQueue(), IntPtr.Zero); - //this.newServerRpcHandler = HandleNewServerRpc; - //this.serverShutdownHandler = HandleServerShutdown; + using (var channelArgs = ChannelOptions.CreateChannelArgs(options)) + { + this.handle = ServerSafeHandle.NewServer(GetCompletionQueue(), channelArgs); + } } /// @@ -141,8 +143,6 @@ namespace Grpc.Core Preconditions.CheckState(!shutdownRequested); shutdownRequested = true; } - - var ctx = BatchContextSafeHandle.Create(); handle.ShutdownAndNotify(HandleServerShutdown); await shutdownTcs.Task; handle.Dispose(); From 5ee11a8e1263eb00f1ddda0cd00570d9dd1681a0 Mon Sep 17 00:00:00 2001 From: Nathaniel Manista Date: Thu, 11 Jun 2015 15:50:13 +0000 Subject: [PATCH 4/6] Stability fixes for python_plugin_test The "normal" timeout is eliminated. The "short" timeout is changed to be the length used in tests that will time out as part of their execution and the "long" timeout is changed to be a ridiculously high value that will have no bearing on passing tests. The "pause" behavior of _ServicerMethods is changed to use a threading.Condition's wait/notify methods rather than busy-sleeping. Tests that used servicer delay to verify that asynchronous calls are not affected by server delay are changed to use servicer pause to verify that asynchronous calls return while the servicer is paused. Busy-sleeping in testHalfDuplexCallWedged is replaced with use of a threading.Condition's wait/notify methods. Fixes https://github.com/grpc/grpc/issues/1900. --- test/compiler/python_plugin_test.py | 132 +++++++++++++++------------- 1 file changed, 71 insertions(+), 61 deletions(-) diff --git a/test/compiler/python_plugin_test.py b/test/compiler/python_plugin_test.py index 653a5ac58c6..0e58d912b9a 100644 --- a/test/compiler/python_plugin_test.py +++ b/test/compiler/python_plugin_test.py @@ -36,6 +36,7 @@ import shutil import subprocess import sys import tempfile +import threading import time import unittest @@ -49,13 +50,13 @@ STUB_IDENTIFIER = 'EarlyAdopterTestServiceStub' SERVER_FACTORY_IDENTIFIER = 'early_adopter_create_TestService_server' STUB_FACTORY_IDENTIFIER = 'early_adopter_create_TestService_stub' -# Timeouts and delays. -SHORT_TIMEOUT = 0.1 -NORMAL_TIMEOUT = 1 -LONG_TIMEOUT = 2 -DOES_NOT_MATTER_DELAY = 0 +# The timeout used in tests of RPCs that are supposed to expire. +SHORT_TIMEOUT = 2 +# The timeout used in tests of RPCs that are not supposed to expire. The +# absurdly large value doesn't matter since no passing execution of this test +# module will ever wait the duration. +LONG_TIMEOUT = 600 NO_DELAY = 0 -LONG_DELAY = 1 # Build mode environment variable set by tools/run_tests/run_tests.py. _build_mode = os.environ['CONFIG'] @@ -64,29 +65,36 @@ _build_mode = os.environ['CONFIG'] class _ServicerMethods(object): def __init__(self, test_pb2, delay): + self._condition = threading.Condition() + self._delay = delay self._paused = False - self._failed = False + self._fail = False self._test_pb2 = test_pb2 - self._delay = delay @contextlib.contextmanager def pause(self): # pylint: disable=invalid-name - self._paused = True + with self._condition: + self._paused = True yield - self._paused = False + with self._condition: + self._paused = False + self._condition.notify_all() @contextlib.contextmanager def fail(self): # pylint: disable=invalid-name - self._failed = True + with self._condition: + self._fail = True yield - self._failed = False + with self._condition: + self._fail = False def _control(self): # pylint: disable=invalid-name - if self._failed: - raise ValueError() + with self._condition: + if self._fail: + raise ValueError() + while self._paused: + self._condition.wait() time.sleep(self._delay) - while self._paused: - time.sleep(0) def UnaryCall(self, request, unused_rpc_context): response = self._test_pb2.SimpleResponse() @@ -147,9 +155,8 @@ def _CreateService(test_pb2, delay): waiting for the service. Args: - test_pb2: the test_pb2 module generated by this test - delay: delay in seconds per response from the servicer - timeout: how long the stub will wait for the servicer by default. + test_pb2: The test_pb2 module generated by this test. + delay: Delay in seconds per response from the servicer. Yields: A (servicer_methods, servicer, stub) three-tuple where servicer_methods is @@ -250,7 +257,7 @@ class PythonPluginTest(unittest.TestCase): if exc.errno != errno.ENOENT: raise - # TODO(atash): Figure out which of theses tests is hanging flakily with small + # TODO(atash): Figure out which of these tests is hanging flakily with small # probability. def testImportAttributes(self): @@ -265,34 +272,33 @@ class PythonPluginTest(unittest.TestCase): def testUpDown(self): import test_pb2 with _CreateService( - test_pb2, DOES_NOT_MATTER_DELAY) as (servicer, stub, unused_server): + test_pb2, NO_DELAY) as (servicer, stub, unused_server): request = test_pb2.SimpleRequest(response_size=13) def testUnaryCall(self): import test_pb2 # pylint: disable=g-import-not-at-top with _CreateService(test_pb2, NO_DELAY) as (methods, stub, unused_server): + timeout = 6 # TODO(issue 2039): LONG_TIMEOUT like the other methods. request = test_pb2.SimpleRequest(response_size=13) - response = stub.UnaryCall(request, NORMAL_TIMEOUT) + response = stub.UnaryCall(request, timeout) expected_response = methods.UnaryCall(request, 'not a real RpcContext!') self.assertEqual(expected_response, response) def testUnaryCallAsync(self): import test_pb2 # pylint: disable=g-import-not-at-top request = test_pb2.SimpleRequest(response_size=13) - with _CreateService(test_pb2, LONG_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): - start_time = time.clock() - response_future = stub.UnaryCall.async(request, LONG_TIMEOUT) - # Check that we didn't block on the asynchronous call. - self.assertGreater(LONG_DELAY, time.clock() - start_time) + # Check that the call does not block waiting for the server to respond. + with methods.pause(): + response_future = stub.UnaryCall.async(request, LONG_TIMEOUT) response = response_future.result() expected_response = methods.UnaryCall(request, 'not a real RpcContext!') self.assertEqual(expected_response, response) def testUnaryCallAsyncExpired(self): import test_pb2 # pylint: disable=g-import-not-at-top - # set the timeout super low... - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): request = test_pb2.SimpleRequest(response_size=13) with methods.pause(): @@ -305,7 +311,7 @@ class PythonPluginTest(unittest.TestCase): def testUnaryCallAsyncCancelled(self): import test_pb2 # pylint: disable=g-import-not-at-top request = test_pb2.SimpleRequest(response_size=13) - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): with methods.pause(): response_future = stub.UnaryCall.async(request, 1) @@ -315,17 +321,17 @@ class PythonPluginTest(unittest.TestCase): def testUnaryCallAsyncFailed(self): import test_pb2 # pylint: disable=g-import-not-at-top request = test_pb2.SimpleRequest(response_size=13) - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): with methods.fail(): - response_future = stub.UnaryCall.async(request, NORMAL_TIMEOUT) + response_future = stub.UnaryCall.async(request, LONG_TIMEOUT) self.assertIsNotNone(response_future.exception()) def testStreamingOutputCall(self): import test_pb2 # pylint: disable=g-import-not-at-top request = _streaming_output_request(test_pb2) with _CreateService(test_pb2, NO_DELAY) as (methods, stub, unused_server): - responses = stub.StreamingOutputCall(request, NORMAL_TIMEOUT) + responses = stub.StreamingOutputCall(request, LONG_TIMEOUT) expected_responses = methods.StreamingOutputCall( request, 'not a real RpcContext!') for expected_response, response in itertools.izip_longest( @@ -337,7 +343,7 @@ class PythonPluginTest(unittest.TestCase): def testStreamingOutputCallExpired(self): import test_pb2 # pylint: disable=g-import-not-at-top request = _streaming_output_request(test_pb2) - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): with methods.pause(): responses = stub.StreamingOutputCall(request, SHORT_TIMEOUT) @@ -349,7 +355,7 @@ class PythonPluginTest(unittest.TestCase): def testStreamingOutputCallCancelled(self): import test_pb2 # pylint: disable=g-import-not-at-top request = _streaming_output_request(test_pb2) - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( unused_methods, stub, unused_server): responses = stub.StreamingOutputCall(request, SHORT_TIMEOUT) next(responses) @@ -362,7 +368,7 @@ class PythonPluginTest(unittest.TestCase): def testStreamingOutputCallFailed(self): import test_pb2 # pylint: disable=g-import-not-at-top request = _streaming_output_request(test_pb2) - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): with methods.fail(): responses = stub.StreamingOutputCall(request, 1) @@ -375,20 +381,19 @@ class PythonPluginTest(unittest.TestCase): def testStreamingInputCall(self): import test_pb2 # pylint: disable=g-import-not-at-top with _CreateService(test_pb2, NO_DELAY) as (methods, stub, unused_server): - response = stub.StreamingInputCall(StreamingInputRequest(test_pb2), - NORMAL_TIMEOUT) + response = stub.StreamingInputCall( + _streaming_input_request_iterator(test_pb2), LONG_TIMEOUT) expected_response = methods.StreamingInputCall( _streaming_input_request_iterator(test_pb2), 'not a real RpcContext!') self.assertEqual(expected_response, response) def testStreamingInputCallAsync(self): import test_pb2 # pylint: disable=g-import-not-at-top - with _CreateService(test_pb2, LONG_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): - start_time = time.clock() - response_future = stub.StreamingInputCall.async( - _streaming_input_request_iterator(test_pb2), LONG_TIMEOUT) - self.assertGreater(LONG_DELAY, time.clock() - start_time) + with methods.pause(): + response_future = stub.StreamingInputCall.async( + _streaming_input_request_iterator(test_pb2), LONG_TIMEOUT) response = response_future.result() expected_response = methods.StreamingInputCall( _streaming_input_request_iterator(test_pb2), 'not a real RpcContext!') @@ -396,8 +401,7 @@ class PythonPluginTest(unittest.TestCase): def testStreamingInputCallAsyncExpired(self): import test_pb2 # pylint: disable=g-import-not-at-top - # set the timeout super low... - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): with methods.pause(): response_future = stub.StreamingInputCall.async( @@ -409,11 +413,12 @@ class PythonPluginTest(unittest.TestCase): def testStreamingInputCallAsyncCancelled(self): import test_pb2 # pylint: disable=g-import-not-at-top - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): with methods.pause(): + timeout = 6 # TODO(issue 2039): LONG_TIMEOUT like the other methods. response_future = stub.StreamingInputCall.async( - _streaming_input_request_iterator(test_pb2), NORMAL_TIMEOUT) + _streaming_input_request_iterator(test_pb2), timeout) response_future.cancel() self.assertTrue(response_future.cancelled()) with self.assertRaises(future.CancelledError): @@ -421,7 +426,7 @@ class PythonPluginTest(unittest.TestCase): def testStreamingInputCallAsyncFailed(self): import test_pb2 # pylint: disable=g-import-not-at-top - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): with methods.fail(): response_future = stub.StreamingInputCall.async( @@ -432,7 +437,7 @@ class PythonPluginTest(unittest.TestCase): import test_pb2 # pylint: disable=g-import-not-at-top with _CreateService(test_pb2, NO_DELAY) as (methods, stub, unused_server): responses = stub.FullDuplexCall( - _full_duplex_request_iterator(test_pb2), NORMAL_TIMEOUT) + _full_duplex_request_iterator(test_pb2), LONG_TIMEOUT) expected_responses = methods.FullDuplexCall( _full_duplex_request_iterator(test_pb2), 'not a real RpcContext!') for expected_response, response in itertools.izip_longest( @@ -444,7 +449,7 @@ class PythonPluginTest(unittest.TestCase): def testFullDuplexCallExpired(self): import test_pb2 # pylint: disable=g-import-not-at-top request_iterator = _full_duplex_request_iterator(test_pb2) - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): with methods.pause(): responses = stub.FullDuplexCall(request_iterator, SHORT_TIMEOUT) @@ -457,7 +462,7 @@ class PythonPluginTest(unittest.TestCase): import test_pb2 # pylint: disable=g-import-not-at-top with _CreateService(test_pb2, NO_DELAY) as (methods, stub, unused_server): request_iterator = _full_duplex_request_iterator(test_pb2) - responses = stub.FullDuplexCall(request_iterator, NORMAL_TIMEOUT) + responses = stub.FullDuplexCall(request_iterator, LONG_TIMEOUT) next(responses) responses.cancel() with self.assertRaises(future.CancelledError): @@ -468,10 +473,10 @@ class PythonPluginTest(unittest.TestCase): def testFullDuplexCallFailed(self): import test_pb2 # pylint: disable=g-import-not-at-top request_iterator = _full_duplex_request_iterator(test_pb2) - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): with methods.fail(): - responses = stub.FullDuplexCall(request_iterator, NORMAL_TIMEOUT) + responses = stub.FullDuplexCall(request_iterator, LONG_TIMEOUT) self.assertIsNotNone(responses) with self.assertRaises(exceptions.ServicerError): next(responses) @@ -480,7 +485,7 @@ class PythonPluginTest(unittest.TestCase): 'forever and fix.') def testHalfDuplexCall(self): import test_pb2 # pylint: disable=g-import-not-at-top - with _CreateService(test_pb2, DOES_NOT_MATTER_DELAY) as ( + with _CreateService(test_pb2, NO_DELAY) as ( methods, stub, unused_server): def half_duplex_request_iterator(): request = test_pb2.StreamingOutputCallRequest() @@ -491,32 +496,37 @@ class PythonPluginTest(unittest.TestCase): request.response_parameters.add(size=3, interval_us=0) yield request responses = stub.HalfDuplexCall( - half_duplex_request_iterator(), NORMAL_TIMEOUT) + half_duplex_request_iterator(), LONG_TIMEOUT) expected_responses = methods.HalfDuplexCall( - HalfDuplexRequest(), 'not a real RpcContext!') + half_duplex_request_iterator(), 'not a real RpcContext!') for check in itertools.izip_longest(expected_responses, responses): expected_response, response = check self.assertEqual(expected_response, response) def testHalfDuplexCallWedged(self): import test_pb2 # pylint: disable=g-import-not-at-top + condition = threading.Condition() wait_cell = [False] @contextlib.contextmanager def wait(): # pylint: disable=invalid-name # Where's Python 3's 'nonlocal' statement when you need it? - wait_cell[0] = True + with condition: + wait_cell[0] = True yield - wait_cell[0] = False + with condition: + wait_cell[0] = False + condition.notify_all() def half_duplex_request_iterator(): request = test_pb2.StreamingOutputCallRequest() request.response_parameters.add(size=1, interval_us=0) yield request - while wait_cell[0]: - time.sleep(0.1) + with condition: + while wait_cell[0]: + condition.wait() with _CreateService(test_pb2, NO_DELAY) as (methods, stub, unused_server): with wait(): responses = stub.HalfDuplexCall( - half_duplex_request_iterator(), NORMAL_TIMEOUT) + half_duplex_request_iterator(), SHORT_TIMEOUT) # half-duplex waits for the client to send all info with self.assertRaises(exceptions.ExpirationError): next(responses) From 74e770d5e8813994cbacd40f84a00588a2624680 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Thu, 11 Jun 2015 09:38:09 -0700 Subject: [PATCH 5/6] Speed up hash checks, crash on first TSAN error --- tools/run_tests/jobset.py | 16 +++++++++++----- tools/run_tests/run_tests.py | 9 +++++++-- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/tools/run_tests/jobset.py b/tools/run_tests/jobset.py index 985b7a7f165..058a30d1ce2 100755 --- a/tools/run_tests/jobset.py +++ b/tools/run_tests/jobset.py @@ -223,6 +223,7 @@ class Jobset(object): self._travis = travis self._cache = cache self._stop_on_failure = stop_on_failure + self._hashes = {} def start(self, spec): """Start a job. Return True on success, False on failure.""" @@ -231,11 +232,15 @@ class Jobset(object): self.reap() if self.cancelled(): return False if spec.hash_targets: - bin_hash = hashlib.sha1() - for fn in spec.hash_targets: - with open(which(fn)) as f: - bin_hash.update(f.read()) - bin_hash = bin_hash.hexdigest() + if spec.identity() in self._hashes: + bin_hash = self._hashes[spec.identity()] + else: + bin_hash = hashlib.sha1() + for fn in spec.hash_targets: + with open(which(fn)) as f: + bin_hash.update(f.read()) + bin_hash = bin_hash.hexdigest() + self._hashes[spec.identity()] = bin_hash should_run = self._cache.should_run(spec.identity(), bin_hash) else: bin_hash = None @@ -266,6 +271,7 @@ class Jobset(object): for job in self._running: job.kill() dead.add(job) + break for job in dead: self._completed += 1 self._running.remove(job) diff --git a/tools/run_tests/run_tests.py b/tools/run_tests/run_tests.py index 32405675b6b..ea40d7e990c 100755 --- a/tools/run_tests/run_tests.py +++ b/tools/run_tests/run_tests.py @@ -313,7 +313,7 @@ _CONFIGS = { 'dbg': SimpleConfig('dbg'), 'opt': SimpleConfig('opt'), 'tsan': SimpleConfig('tsan', environ={ - 'TSAN_OPTIONS': 'suppressions=tools/tsan_suppressions.txt'}), + 'TSAN_OPTIONS': 'suppressions=tools/tsan_suppressions.txt:halt_on_error=1'}), 'msan': SimpleConfig('msan'), 'ubsan': SimpleConfig('ubsan'), 'asan': SimpleConfig('asan', environ={ @@ -449,6 +449,7 @@ class TestCache(object): def __init__(self, use_cache_results): self._last_successful_run = {} self._use_cache_results = use_cache_results + self._last_save = time.time() def should_run(self, cmdline, bin_hash): if cmdline not in self._last_successful_run: @@ -461,7 +462,8 @@ class TestCache(object): def finished(self, cmdline, bin_hash): self._last_successful_run[cmdline] = bin_hash - self.save() + if time.time() - self._last_save > 1: + self.save() def dump(self): return [{'cmdline': k, 'hash': v} @@ -473,6 +475,7 @@ class TestCache(object): def save(self): with open('.run_tests_cache', 'w') as f: f.write(json.dumps(self.dump())) + self._last_save = time.time() def maybe_load(self): if os.path.exists('.run_tests_cache'): @@ -515,6 +518,8 @@ def _build_and_run(check_cancelled, newline_on_success, travis, cache): for antagonist in antagonists: antagonist.kill() + if cache: cache.save() + return 0 From 4ec975df616feb5481f5c8184854413ef8bd229f Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Thu, 11 Jun 2015 17:24:15 -0700 Subject: [PATCH 6/6] unknown method handler no longer needs to complete request stream --- src/csharp/Grpc.Core/Internal/AsyncCallServer.cs | 1 + src/csharp/Grpc.Core/Internal/ServerCallHandler.cs | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs b/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs index db1b86937f8..4f510ba40ac 100644 --- a/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs +++ b/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs @@ -107,6 +107,7 @@ namespace Grpc.Core.Internal call.StartSendStatusFromServer(status, HandleHalfclosed); halfcloseRequested = true; + readingDone = true; sendCompletionDelegate = completionDelegate; } } diff --git a/src/csharp/Grpc.Core/Internal/ServerCallHandler.cs b/src/csharp/Grpc.Core/Internal/ServerCallHandler.cs index f494d9e0ffc..c0e5bae13f0 100644 --- a/src/csharp/Grpc.Core/Internal/ServerCallHandler.cs +++ b/src/csharp/Grpc.Core/Internal/ServerCallHandler.cs @@ -267,8 +267,6 @@ namespace Grpc.Core.Internal var responseStream = new ServerResponseStream(asyncCall); await responseStream.WriteStatusAsync(new Status(StatusCode.Unimplemented, "No such method.")); - // TODO(jtattermusch): if we don't read what client has sent, the server call never gets disposed. - await requestStream.ToList(); await finishedTask; } }