-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extracted Serializer to better manage message contract types #136
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,6 @@ | |
using System.Security.Cryptography.X509Certificates; | ||
using System.Threading.Tasks; | ||
using FluentAssertions; | ||
using FluentAssertions.Common; | ||
using Halibut.ServiceModel; | ||
using Halibut.Tests.TestServices; | ||
using NUnit.Framework; | ||
|
@@ -65,6 +64,7 @@ public void OctopusCanSendMessagesToListeningTentacle() | |
public void OctopusCanSendMessagesToPollingTentacle() | ||
{ | ||
var services = GetDelegateServiceFactory(); | ||
services.Register<ISupportedServices>(() => new SupportedServices()); | ||
using (var octopus = new HalibutRuntime(Certificates.Octopus)) | ||
using (var tentaclePolling = new HalibutRuntime(services, Certificates.TentaclePolling)) | ||
{ | ||
|
@@ -73,10 +73,16 @@ public void OctopusCanSendMessagesToPollingTentacle() | |
|
||
tentaclePolling.Poll(new Uri("poll://SQ-TENTAPOLL"), new ServiceEndPoint(new Uri("https://localhost:" + octopusPort), Certificates.OctopusPublicThumbprint)); | ||
|
||
// This is here to exercise the path there the Listener's (web socket) handle loop has the protocol (with type serializer) built before the type is registered | ||
var echo = octopus.CreateClient<IEchoService>("poll://SQ-TENTAPOLL", Certificates.TentaclePollingPublicThumbprint); | ||
for (var i = 0; i < 2000; i++) | ||
echo.SayHello("Deploy package A").Should().Be("Deploy package A" + "..."); // This must come before CreateClient<ISupportedServices> for the situation to occur | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a easy call either way, but I wonder if it's worth moving this into its own test? |
||
|
||
//Thread.Sleep(TimeSpan.FromSeconds(12)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line can be removed |
||
var svc = octopus.CreateClient<ISupportedServices>("poll://SQ-TENTAPOLL", Certificates.TentaclePollingPublicThumbprint); | ||
for (var i = 1; i < 100; i++) | ||
{ | ||
echo.SayHello("Deploy package A" + i).Should().Be("Deploy package A" + i + "..."); | ||
var i1 = i; | ||
svc.GetLocation(new MapLocation { Latitude = -i, Longitude = i }).Should().Match<MapLocation>(x => x.Latitude == i1 && x.Longitude == -i1); | ||
} | ||
} | ||
} | ||
|
@@ -100,11 +106,15 @@ public void OctopusCanSendMessagesToWebSocketPollingTentacle() | |
|
||
tentaclePolling.Poll(new Uri("poll://SQ-TENTAPOLL"), new ServiceEndPoint(new Uri($"wss://localhost:{octopusPort}/Halibut"), Certificates.SslThumbprint)); | ||
|
||
var echo = octopus.CreateClient<ISupportedServices>("poll://SQ-TENTAPOLL", Certificates.TentaclePollingPublicThumbprint); | ||
// This is here to exercise the path there the Listener's (web socket) handle loop has the protocol (with type serializer) built before the type is registered | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there ➡ where |
||
var echo = octopus.CreateClient<IEchoService>("poll://SQ-TENTAPOLL", Certificates.TentaclePollingPublicThumbprint); | ||
echo.SayHello("Deploy package A").Should().Be("Deploy package A" + "..."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above regarding possibly making another test for this. |
||
|
||
var svc = octopus.CreateClient<ISupportedServices>("poll://SQ-TENTAPOLL", Certificates.TentaclePollingPublicThumbprint); | ||
for (var i = 1; i < 100; i++) | ||
{ | ||
var i1 = i; | ||
echo.GetLocation(new MapLocation { Latitude = -i, Longitude = i }).Should().Match<MapLocation>(x => x.Latitude == i1 && x.Longitude == -i1); | ||
svc.GetLocation(new MapLocation { Latitude = -i, Longitude = i }).Should().Match<MapLocation>(x => x.Latitude == i1 && x.Longitude == -i1); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,8 +30,7 @@ public class HalibutRuntime : IHalibutRuntime | |
readonly PollingClientCollection pollingClients = new PollingClientCollection(); | ||
string friendlyHtmlPageContent = DefaultFriendlyHtmlPageContent; | ||
Dictionary<string, string> friendlyHtmlPageHeaders = new Dictionary<string, string>(); | ||
readonly IServiceFactory serviceFactory; | ||
readonly HashSet<Type> clientTypes = new HashSet<Type>(); | ||
readonly MessageSerializer messageSerializer = new MessageSerializer(); | ||
|
||
public HalibutRuntime(X509Certificate2 serverCertificate) : this(new NullServiceFactory(), serverCertificate, new DefaultTrustProvider()) | ||
{ | ||
|
@@ -49,7 +48,7 @@ public HalibutRuntime(IServiceFactory serviceFactory, X509Certificate2 serverCer | |
{ | ||
this.serverCertificate = serverCertificate; | ||
this.trustProvider = trustProvider; | ||
this.serviceFactory = serviceFactory; | ||
messageSerializer.AddToMessageContract(serviceFactory.RegisteredServiceTypes.ToArray()); | ||
invoker = new ServiceInvoker(serviceFactory); | ||
} | ||
|
||
|
@@ -75,36 +74,31 @@ public int Listen(int port) | |
|
||
return Listen(new IPEndPoint(ipAddress, port)); | ||
} | ||
|
||
IEnumerable<Type> AllProtocolTypes() | ||
{ | ||
foreach (var clientType in clientTypes) | ||
{ | ||
yield return clientType; | ||
} | ||
|
||
foreach (var serviceType in serviceFactory.RegisteredServiceTypes) | ||
{ | ||
yield return serviceType; | ||
} | ||
} | ||
|
||
ExchangeProtocolBuilder ExchangeProtocolBuilder() | ||
{ | ||
return (stream, log) => new MessageExchangeProtocol(new MessageExchangeStream(stream, AllProtocolTypes(), log), log); | ||
return (stream, log) => new MessageExchangeProtocol(new MessageExchangeStream(stream, messageSerializer, log), log); | ||
} | ||
|
||
public int Listen(IPEndPoint endpoint) | ||
{ | ||
var listener = new SecureListener(endpoint, serverCertificate, ExchangeProtocolBuilder(), HandleMessage, IsTrusted, logs, () => friendlyHtmlPageContent, () => friendlyHtmlPageHeaders, HandleUnauthorizedClientConnect); | ||
listeners.Add(listener); | ||
lock (listeners) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that any calls to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the time frame needed for this fix though, it might be worth just adding a comment saying why the locks are needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree in principle, and yes it shouldn't be too hard... but for now, will skip |
||
{ | ||
listeners.Add(listener); | ||
} | ||
|
||
return listener.Start(); | ||
} | ||
|
||
public void ListenWebSocket(string endpoint) | ||
{ | ||
var listener = new SecureWebSocketListener(endpoint, serverCertificate, ExchangeProtocolBuilder(), HandleMessage, IsTrusted, logs, () => friendlyHtmlPageContent, () => friendlyHtmlPageHeaders, HandleUnauthorizedClientConnect); | ||
listeners.Add(listener); | ||
lock (listeners) | ||
{ | ||
listeners.Add(listener); | ||
} | ||
|
||
listener.Start(); | ||
} | ||
|
||
|
@@ -177,7 +171,8 @@ public TService CreateClient<TService>(ServiceEndPoint endpoint) | |
|
||
public TService CreateClient<TService>(ServiceEndPoint endpoint, CancellationToken cancellationToken) | ||
{ | ||
clientTypes.Add(typeof(TService)); | ||
messageSerializer.AddToMessageContract(typeof(TService)); | ||
|
||
#if HAS_REAL_PROXY | ||
return (TService)new HalibutProxy(SendOutgoingRequest, typeof(TService), endpoint, cancellationToken).GetTransparentProxy(); | ||
#else | ||
|
@@ -254,9 +249,12 @@ void DisconnectFromAllListeners(IReadOnlyCollection<string> thumbprints) | |
|
||
void DisconnectFromAllListeners(string thumbprint) | ||
{ | ||
foreach (var secureListener in listeners.OfType<SecureListener>()) | ||
lock (listeners) | ||
{ | ||
secureListener.Disconnect(thumbprint); | ||
foreach (var secureListener in listeners.OfType<SecureListener>()) | ||
{ | ||
secureListener.Disconnect(thumbprint); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -290,9 +288,12 @@ public void Dispose() | |
{ | ||
pollingClients.Dispose(); | ||
connectionManager.Dispose(); | ||
foreach (var listener in listeners) | ||
lock (listeners) | ||
{ | ||
listener.Dispose(); | ||
foreach (var listener in listeners) | ||
{ | ||
listener?.Dispose(); | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,10 @@ public void Register<TContract>(Func<TContract> implementation) | |
{ | ||
var serviceType = typeof(TContract); | ||
services.Add(serviceType.Name, () => implementation()); | ||
serviceTypes.Add(serviceType); | ||
lock (serviceTypes) | ||
{ | ||
serviceTypes.Add(serviceType); | ||
} | ||
} | ||
|
||
public IServiceLease CreateService(string serviceName) | ||
|
@@ -24,8 +27,7 @@ public IServiceLease CreateService(string serviceName) | |
|
||
Func<object> GetService(string name) | ||
{ | ||
Func<object> result; | ||
if (!services.TryGetValue(name, out result)) | ||
if (!services.TryGetValue(name, out var result)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So much cleaner... 👍 |
||
{ | ||
throw new Exception("Service not found: " + name); | ||
} | ||
|
@@ -39,7 +41,16 @@ static IServiceLease CreateService(Func<object> serviceBuilder) | |
return new Lease(service); | ||
} | ||
|
||
public IReadOnlyList<Type> RegisteredServiceTypes => serviceTypes.ToList(); | ||
public IReadOnlyList<Type> RegisteredServiceTypes | ||
{ | ||
get | ||
{ | ||
lock (serviceTypes) | ||
{ | ||
return serviceTypes.ToList(); | ||
} | ||
} | ||
} | ||
|
||
#region Nested type: Lease | ||
|
||
|
@@ -52,16 +63,13 @@ public Lease(object service) | |
this.service = service; | ||
} | ||
|
||
public object Service | ||
{ | ||
get { return service; } | ||
} | ||
public object Service => service; | ||
|
||
public void Dispose() | ||
{ | ||
if (service is IDisposable) | ||
if (service is IDisposable disposable) | ||
{ | ||
((IDisposable)service).Dispose(); | ||
disposable.Dispose(); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
using System.IO; | ||
|
||
namespace Halibut.Transport.Protocol | ||
{ | ||
public interface IMessageSerializer | ||
{ | ||
void WriteMessage<T>(Stream stream, T message); | ||
|
||
T ReadMessage<T>(Stream stream); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there ➡ where