Skip to content

Commit

Permalink
Welcome?BackTransportAbortHandler - reverting TransportAbortHandler r…
Browse files Browse the repository at this point in the history
…emoval

We found that removing TransportAbortHandler caused a few issues and there might be more we did not find. Reverting change to avoid breaks.
  • Loading branch information
moozzyk committed Aug 7, 2014
1 parent 7d43746 commit 52b3fe0
Show file tree
Hide file tree
Showing 14 changed files with 240 additions and 91 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,9 @@
<Compile Include="..\Microsoft.AspNet.SignalR.Client\Infrastructure\StartException.cs">
<Link>Infrastructure\StartException.cs</Link>
</Compile>
<Compile Include="..\Microsoft.AspNet.SignalR.Client\Infrastructure\TransportAbortHandler.cs">
<Link>Infrastructure\TransportAbortHandler.cs</Link>
</Compile>
<Compile Include="..\Microsoft.AspNet.SignalR.Client\Infrastructure\TransportInitializationHandler.cs">
<Link>Infrastructure\TransportInitializationHandler.cs</Link>
</Compile>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,7 @@ private void WebsocketClosed(IWebSocket webSocket, WebSocketClosedEventArgs even

DisposeSocket();

// either abort has been sent or transport has been disposed - do not reconnect
if (Finished)
if (AbortHandler.TryCompleteAbort())
{
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@
<Compile Include="..\Microsoft.AspNet.SignalR.Client\Infrastructure\StartException.cs">
<Link>Infrastructure\StartException.cs</Link>
</Compile>
<Compile Include="..\Microsoft.AspNet.SignalR.Client\Infrastructure\TransportAbortHandler.cs">
<Link>Infrastructure\TransportAbortHandler.cs</Link>
</Compile>
<Compile Include="..\Microsoft.AspNet.SignalR.Client\Infrastructure\TransportInitializationHandler.cs">
<Link>Infrastructure\TransportInitializationHandler.cs</Link>
</Compile>
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.AspNet.SignalR.Client/Connection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ public void Stop(TimeSpan timeout)
// Dispose the heart beat monitor so we don't fire notifications when waiting to abort
Monitor.Dispose();

_transport.Abort(this, _connectionData);
_transport.Abort(this, timeout, _connectionData);

Disconnect();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. See License.md in the project root for license information.

using System;
using System.Threading;
using Microsoft.AspNet.SignalR.Client.Http;

namespace Microsoft.AspNet.SignalR.Client.Infrastructure
{
public class TransportAbortHandler : IDisposable
{
// The transport name
private readonly string _transportName;

private readonly IHttpClient _httpClient;

// Used to complete the synchronous call to Abort()
private ManualResetEvent _abortResetEvent = new ManualResetEvent(initialState: false);

// Used to indicate whether Abort() has been called
private bool _startedAbort;
// Used to ensure that Abort() runs effectively only once
// The _abortLock subsumes the _disposeLock and can be held upwards of 30 seconds
private readonly object _abortLock = new object();

// Used to ensure the _abortResetEvent.Set() isn't called after disposal
private bool _disposed;
// Used to make checking _disposed and calling _abortResetEvent.Set() thread safe
private readonly object _disposeLock = new object();

public TransportAbortHandler(IHttpClient httpClient, string transportName)
{
_httpClient = httpClient;
_transportName = transportName;
}

public virtual void Abort(IConnection connection, TimeSpan timeout, string connectionData)
{
if (connection == null)
{
throw new ArgumentNullException("connection");
}

// Save the connection.ConnectionToken since race issue that connection.ConnectionToken can be set to null in different thread
var connectionToken = connection.ConnectionToken;

if (connectionToken == null)
{
connection.Trace(TraceLevels.Messages, "Connection already disconnected, skipping abort.");
return;
}

// Abort should never complete before any of its previous calls
lock (_abortLock)
{
if (_disposed)
{
return;
}

// Ensure that an abort request is only made once
if (!_startedAbort)
{
_startedAbort = true;

var url = UrlBuilder.BuildAbort(connection, _transportName, connectionData);

_httpClient.Post(url, connection.PrepareRequest, isLongRunning: false)
.Catch((ex, state) =>
{
// If there's an error making an http request set the reset event
((TransportAbortHandler)state).CompleteAbort();
},
this,
connection);

if (!_abortResetEvent.WaitOne(timeout))
{
connection.Trace(TraceLevels.Events, "Abort never fired");
}
}
}
}

public void CompleteAbort()
{
lock (_disposeLock)
{
if (!_disposed)
{
// Make any future calls to Abort() no-op
// Abort might still run, but any ongoing aborts will immediately complete
_startedAbort = true;
// Ensure any ongoing calls to Abort() complete
_abortResetEvent.Set();
}
}
}

public bool TryCompleteAbort()
{
// Make sure we don't Set a disposed ManualResetEvent
lock (_disposeLock)
{
if (_disposed)
{
// Don't try to continue receiving messages if the transport is disposed
return true;
}
else if (_startedAbort)
{
_abortResetEvent.Set();
return true;
}
else
{
return false;
}
}
}

protected virtual void Dispose(bool disposing)
{
if (disposing)
{
// Wait for any ongoing aborts to complete
// In practice, any aborts should have finished by the time Dispose is called
lock (_abortLock)
{
lock (_disposeLock)
{
if (!_disposed)
{
_abortResetEvent.Dispose();
_disposed = true;
}
}
}
}
}

public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
<Compile Include="Infrastructure\ErrorExtensions.cs" />
<Compile Include="Infrastructure\SignalRError.cs" />
<Compile Include="Infrastructure\StartException.cs" />
<Compile Include="Infrastructure\TransportAbortHandler.cs" />
<Compile Include="Infrastructure\TransportInitializationHandler.cs" />
<Compile Include="Infrastructure\UrlBuilder.cs" />
<Compile Include="Infrastructure\UrlEncoder.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,11 @@ public Task Send(IConnection connection, string data, string connectionData)
return _transport.Send(connection, data, connectionData);
}

public void Abort(IConnection connection, string connectionData)
public void Abort(IConnection connection, TimeSpan timeout, string connectionData)
{
if (_transport != null)
{
_transport.Abort(connection, connectionData);
_transport.Abort(connection, timeout, connectionData);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,16 @@ public abstract class ClientTransportBase : IClientTransport
private readonly IHttpClient _httpClient;
private readonly string _transportName;
private readonly TransportHelper _transportHelper;
private int _finished = 0;
private readonly TransportAbortHandler _abortHandler;
private bool _finished = false;

[SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Disposed in the Dispose method.")]
protected ClientTransportBase(IHttpClient httpClient, string transportName)
: this(httpClient, transportName, new TransportHelper())
: this(httpClient, transportName, new TransportHelper(), new TransportAbortHandler(httpClient, transportName))
{
}

internal ClientTransportBase(IHttpClient httpClient, string transportName, TransportHelper transportHelper)
internal ClientTransportBase(IHttpClient httpClient, string transportName, TransportHelper transportHelper, TransportAbortHandler abortHandler)
{
if (httpClient == null)
{
Expand All @@ -36,10 +37,12 @@ internal ClientTransportBase(IHttpClient httpClient, string transportName, Trans
}

Debug.Assert(transportHelper != null, "transportHelper is null");
Debug.Assert(abortHandler != null, "abortHandler is null");

_httpClient = httpClient;
_transportName = transportName;
_transportHelper = transportHelper;
_abortHandler = abortHandler;
}

protected IHttpClient HttpClient
Expand All @@ -49,7 +52,12 @@ protected IHttpClient HttpClient

protected TransportHelper TransportHelper
{
get { return _transportHelper; }
get { return _transportHelper; }
}

protected TransportAbortHandler AbortHandler
{
get { return _abortHandler; }
}

/// <summary>
Expand All @@ -64,7 +72,7 @@ public string Name

public virtual Task<NegotiationResponse> Negotiate(IConnection connection, string connectionData)
{
if (Finished)
if(_finished)
{
throw new InvalidOperationException(Resources.Error_TransportCannotBeReused);
}
Expand All @@ -76,39 +84,10 @@ public virtual Task<NegotiationResponse> Negotiate(IConnection connection, strin

public abstract Task Send(IConnection connection, string data, string connectionData);

public virtual void Abort(IConnection connection, string connectionData)
{
if (connection == null)
{
throw new ArgumentNullException("connection");
}

// Save the connection.ConnectionToken since race issue that connection.ConnectionToken can be set to null in different thread
var connectionToken = connection.ConnectionToken;

if (connectionToken == null)
{
connection.Trace(TraceLevels.Messages, "Connection already disconnected, skipping abort.");
_finished = 1;
return;
}

if (Interlocked.Exchange(ref _finished, 1) == 0)
{
var url = UrlBuilder.BuildAbort(connection, _transportName, connectionData);

_httpClient.Post(url, connection.PrepareRequest, isLongRunning: false)
.Catch(
(ex, state) => connection.Trace(TraceLevels.Messages, "Sending abort failed due to {0}", ex),
this,
connection);
}
}

// internal for testing
protected internal bool Finished
public virtual void Abort(IConnection connection, TimeSpan timeout, string connectionData)
{
get { return _finished != 0; }
_finished = true;
AbortHandler.Abort(connection, timeout, connectionData);
}

public abstract void LostConnection(IConnection connection);
Expand All @@ -121,7 +100,11 @@ public void Dispose()

protected virtual void Dispose(bool disposing)
{
_finished = 1;
if (disposing)
{
_finished = true;
_abortHandler.Dispose();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public interface IClientTransport : IDisposable
Task<NegotiationResponse> Negotiate(IConnection connection, string connectionData);
Task Start(IConnection connection, string connectionData, CancellationToken disconnectToken);
Task Send(IConnection connection, string data, string connectionData);
void Abort(IConnection connection, string connectionData);
void Abort(IConnection connection, TimeSpan timeout, string connectionData);

void LostConnection(IConnection connection);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ private void PollingSetup(IConnection connection,

_requestHandler.OnAfterPoll = exception =>
{
if (Finished)
if (AbortHandler.TryCompleteAbort())
{
// Abort() was called, so don't reconnect
_requestHandler.Stop();
Expand All @@ -206,6 +206,10 @@ private void PollingSetup(IConnection connection,
_requestHandler.OnAbort += _ =>
{
disconnectRegistration.Dispose();
// Complete any ongoing calls to Abort()
// If someone calls Abort() later, have it no-op
AbortHandler.CompleteAbort();
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,15 @@ internal void OpenConnection(IConnection connection,
esCancellationRegistration.Dispose();
response.Dispose();
if (!_stop && !Finished)
if (_stop)
{
AbortHandler.CompleteAbort();
}
else if (AbortHandler.TryCompleteAbort())
{
// Abort() was called, so don't reconnect
}
else
{
Reconnect(connection, data, disconnectToken);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@
<Compile Include="..\Microsoft.AspNet.SignalR.Client\Infrastructure\StartException.cs">
<Link>Infrastructure\StartException.cs</Link>
</Compile>
<Compile Include="..\Microsoft.AspNet.SignalR.Client\Infrastructure\TransportAbortHandler.cs">
<Link>Infrastructure\TransportAbortHandler.cs</Link>
</Compile>
<Compile Include="..\Microsoft.AspNet.SignalR.Client\Infrastructure\TransportInitializationHandler.cs">
<Link>Infrastructure\TransportInitializationHandler.cs</Link>
</Compile>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,12 @@ internal virtual void OnClose()
{
_connection.Trace(TraceLevels.Events, "WS: OnClose()");

if (_disconnectToken.IsCancellationRequested || Finished)
if (_disconnectToken.IsCancellationRequested)
{
return;
}

if (AbortHandler.TryCompleteAbort())
{
return;
}
Expand Down
Loading

0 comments on commit 52b3fe0

Please sign in to comment.