Skip to content
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

fix auto reconnect after stop bug #707

Merged
merged 19 commits into from
Nov 4, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ public Task StopAsync()
{
Log.UnexpectedExceptionInStop(Logger, ConnectionId, ex);
}

return Task.CompletedTask;
}

Expand Down Expand Up @@ -388,7 +388,7 @@ private async Task<bool> ReceiveHandshakeResponseAsync(PipeReader input, Cancell
{
Log.HandshakeError(Logger, handshakeResponse.ErrorMessage, ConnectionId);
}

return false;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,13 @@ internal abstract class ServiceConnectionContainerBase : IServiceConnectionConta
private readonly object _statusLock = new object();

private readonly AckHandler _ackHandler;

private volatile List<IServiceConnection> _fixedServiceConnections;

private volatile ServiceConnectionStatus _status;

private volatile bool _terminated = false;

protected ILogger Logger { get; }

protected List<IServiceConnection> FixedServiceConnections
Expand Down Expand Up @@ -88,7 +91,7 @@ protected ServiceConnectionContainerBase(IServiceConnectionFactory serviceConnec
else
{
initial = new List<IServiceConnection>(initialConnections);
foreach(var conn in initial)
foreach (var conn in initial)
{
conn.ConnectionStatusChanged += OnConnectionStatusChanged;
}
Expand All @@ -112,7 +115,11 @@ public Task StartAsync()
return Task.WhenAll(FixedServiceConnections.Select(c => StartCoreAsync(c)));
}

public virtual Task StopAsync() => Task.WhenAll(FixedServiceConnections.Select(c => c.StopAsync()));
public virtual Task StopAsync()
{
_terminated = true;
return Task.WhenAll(FixedServiceConnections.Select(c => c.StopAsync()));
}

/// <summary>
/// Start and manage the whole connection lifetime
Expand Down Expand Up @@ -155,6 +162,11 @@ protected virtual async Task OnConnectionComplete(IServiceConnection serviceConn
throw new ArgumentNullException(nameof(serviceConnection));
}

if (_terminated)
{
return;
}

serviceConnection.ConnectionStatusChanged -= OnConnectionStatusChanged;

if (serviceConnection.Status == ServiceConnectionStatus.Connected)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@ internal enum ServiceConnectionStatus
Inited,
Disconnected,
Connecting,
Connected
Connected,
}
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Runtime.CompilerServices;

[assembly : InternalsVisibleTo("Microsoft.Azure.SignalR.Management.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
[assembly : InternalsVisibleTo("Microsoft.Azure.SignalR.Management.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
[assembly : InternalsVisibleTo("Microsoft.Azure.SignalR.E2ETests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
10 changes: 8 additions & 2 deletions src/Microsoft.Azure.SignalR.Management/ServiceHubContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,19 @@

using System.Threading.Tasks;
using Microsoft.AspNetCore.SignalR;
using Microsoft.Azure.SignalR.Common.ServiceConnections;
using Microsoft.Extensions.DependencyInjection;

namespace Microsoft.Azure.SignalR.Management
{
internal class ServiceHubContext : IServiceHubContext
{
private IHubContext<Hub> _hubContext;
private ServiceProvider _serviceProvider;
private readonly IHubContext<Hub> _hubContext;

private readonly ServiceProvider _serviceProvider;

// for test only
internal IServiceConnectionContainer ConnectionContainer { get; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we don't move this property into its relevant TestServiceHubContext class?

And even if we have to define an internal property here, we can use

get => _serviceProvider.GetService<IServiceConnectionContainer>()

instead of initializing it in the constructor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

init in .ctor is better as it is only set once

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we create a inner class and make all metheds and fields only placed in this inner class? this makes the outer class clean.


public IHubClients Clients => _hubContext.Clients;

Expand All @@ -23,6 +28,7 @@ public ServiceHubContext(IHubContext<Hub> hubContext, IHubLifetimeManagerForUser
_hubContext = hubContext;
UserGroups = new UserGroupsManager(lifetimeManager);
_serviceProvider = serviceProvider;
ConnectionContainer = _serviceProvider.GetService<IServiceConnectionContainer>();
}

public async Task DisposeAsync()
Expand Down
6 changes: 5 additions & 1 deletion src/Microsoft.Azure.SignalR.Management/ServiceManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@ public async Task<IServiceHubContext> CreateHubContextAsync(string hubName, ILog
var clientConnectionFactory = new ClientConnectionFactory();
ConnectionDelegate connectionDelegate = connectionContext => Task.CompletedTask;
var serviceConnectionFactory = new ServiceConnectionFactory(serviceProtocol, clientConnectionManager, connectionFactory, loggerFactory, connectionDelegate, clientConnectionFactory);
var weakConnectionContainer = new WeakServiceConnectionContainer(serviceConnectionFactory, _serviceManagerOptions.ConnectionCount, new HubServiceEndpoint(hubName, _endpointProvider, _endpoint), NullLogger.Instance);
var weakConnectionContainer = new WeakServiceConnectionContainer(
serviceConnectionFactory,
_serviceManagerOptions.ConnectionCount,
new HubServiceEndpoint(hubName, _endpointProvider, _endpoint),
loggerFactory?.CreateLogger(nameof(WeakServiceConnectionContainer)) ?? NullLogger.Instance);

var serviceCollection = new ServiceCollection();
serviceCollection.AddSignalRCore();
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.Azure.SignalR.Management/TaskExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ internal static class TaskExtensions
{
private static readonly TimeSpan _defaultTimeout = TimeSpan.FromMinutes(5);

public static async Task OrTimeout(this Task task, CancellationToken cancellationToken = default)
public static async Task OrTimeout(this Task task, CancellationToken cancellationToken)
{
if (cancellationToken == default)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ await RunTestCore(clientEndpoint, clientAccessTokens,
internal async Task SendToConnectionTest(ServiceTransportType serviceTransportType, string appName)
{
var testServer = _testServerFactory.Create(TestOutputHelper);
await testServer.StartAsync(new Dictionary<string, string>{ [TestStartup.ApplicationName] = appName });
await testServer.StartAsync(new Dictionary<string, string> { [TestStartup.ApplicationName] = appName });

var task = testServer.HubConnectionManager.WaitForConnectionCountAsync(1);

Expand All @@ -171,7 +171,7 @@ internal async Task SendToConnectionTest(ServiceTransportType serviceTransportTy
try
{
await RunTestCore(clientEndpoint, clientAccessTokens,
async () =>
async () =>
{
var connectionId = await task.OrTimeout();
await serviceHubContext.Clients.Client(connectionId).SendAsync(MethodName, Message);
Expand Down Expand Up @@ -217,6 +217,28 @@ await RunTestCore(clientEndpoint, clientAccessTokens,
}
}

[ConditionalFact]
[SkipIfConnectionStringNotPresent]
internal async Task StopServiceHubContextTest()
{
using (StartVerifiableLog(out var loggerFactory, LogLevel.Debug, expectedErrors: context => context.EventId == new EventId(2, "EndpointOffline")))
{
var serviceManager = new ServiceManagerBuilder()
.WithOptions(o =>
{
o.ConnectionString = TestConfiguration.Instance.ConnectionString;
o.ConnectionCount = 1;
o.ServiceTransportType = ServiceTransportType.Persistent;
})
.Build();
var serviceHubContext = await serviceManager.CreateHubContextAsync("hub", loggerFactory);
var connectionContainer = ((ServiceHubContext)serviceHubContext).ConnectionContainer;
await serviceHubContext.DisposeAsync();
await Task.Delay(500);
Assert.Equal(ServiceConnectionStatus.Disconnected, connectionContainer.Status);
}
}

private static IDictionary<string, List<string>> GenerateUserGroupDict(IList<string> userNames, IList<string> groupNames)
{
return (from i in Enumerable.Range(0, userNames.Count)
Expand Down