Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Commit a138c47

Browse files
moozzykmoozzyk
committed
Exceptions from user's event handlers should be caught and logged
Otherwise they can spoil event queue and make the client not raise the Received event anymore Fixes: #818
1 parent 9277523 commit a138c47

File tree

3 files changed

+170
-15
lines changed

3 files changed

+170
-15
lines changed

‎src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -152,16 +152,25 @@ private async Task StartAsyncInternal()
152152
if (Interlocked.CompareExchange(ref _connectionState, ConnectionState.Connected, ConnectionState.Connecting)
153153
== ConnectionState.Connecting)
154154
{
155-
var ignore = _eventQueue.Enqueue(() =>
155+
_ = _eventQueue.Enqueue(async () =>
156156
{
157157
_logger.RaiseConnected(_connectionId);
158158

159-
Connected?.Invoke();
160-
161-
return Task.CompletedTask;
159+
var connectedEventHandler = Connected;
160+
if (connectedEventHandler != null)
161+
{
162+
try
163+
{
164+
await connectedEventHandler.Invoke();
165+
}
166+
catch (Exception ex)
167+
{
168+
_logger.ExceptionThrownFromEventHandler(_connectionId, nameof(Connected), ex);
169+
}
170+
}
162171
});
163172

164-
ignore = Input.Completion.ContinueWith(async t =>
173+
_ = Input.Completion.ContinueWith(async t =>
165174
{
166175
Interlocked.Exchange(ref _connectionState, ConnectionState.Disconnected);
167176

@@ -183,9 +192,18 @@ private async Task StartAsyncInternal()
183192

184193
_logger.RaiseClosed(_connectionId);
185194

186-
Closed?.Invoke(t.IsFaulted ? t.Exception.InnerException : null);
187-
188-
return Task.CompletedTask;
195+
var closedEventHandler = Closed;
196+
if (closedEventHandler != null)
197+
{
198+
try
199+
{
200+
await closedEventHandler.Invoke(t.IsFaulted ? t.Exception.InnerException : null);
201+
}
202+
catch (Exception ex)
203+
{
204+
_logger.ExceptionThrownFromEventHandler(_connectionId, nameof(Closed), ex);
205+
}
206+
}
189207
});
190208

191209
// start receive loop only after the Connected event was raised to
@@ -331,19 +349,22 @@ private async Task ReceiveAsync()
331349
if (Input.TryRead(out var buffer))
332350
{
333351
_logger.ScheduleReceiveEvent(_connectionId);
334-
_ = _eventQueue.Enqueue(() =>
352+
_ = _eventQueue.Enqueue(async () =>
335353
{
336354
_logger.RaiseReceiveEvent(_connectionId);
337355

338-
// Making a copy of the Received handler to ensure that its not null
339-
// Can't use the ? operator because we specifically want to check if the handler is null
340356
var receivedHandler = Received;
341357
if (receivedHandler != null)
342358
{
343-
return receivedHandler(buffer);
359+
try
360+
{
361+
await receivedHandler(buffer);
362+
}
363+
catch (Exception ex)
364+
{
365+
_logger.ExceptionThrownFromEventHandler(_connectionId, nameof(Received), ex);
366+
}
344367
}
345-
346-
return Task.CompletedTask;
347368
});
348369
}
349370
else

‎src/Microsoft.AspNetCore.Sockets.Client.Http/Internal/SocketClientLoggerExtensions.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,10 @@ internal static class SocketClientLoggerExtensions
150150
private static readonly Action<ILogger, DateTime, string, Exception> _stoppingClient =
151151
LoggerMessage.Define<DateTime, string>(LogLevel.Information, 18, "{time}: Connection Id {connectionId}: Stopping client.");
152152

153+
private static readonly Action<ILogger, DateTime, string, string, Exception> _exceptionThrownFromHandler =
154+
LoggerMessage.Define<DateTime, string, string>(LogLevel.Error, 19, "{time}: Connection Id {connectionId}: An exception was thrown from {eventHandlerName} event handler.");
155+
156+
153157
public static void StartTransport(this ILogger logger, string connectionId, TransferMode transferMode)
154158
{
155159
if (logger.IsEnabled(LogLevel.Information))
@@ -509,5 +513,13 @@ public static void StoppingClient(this ILogger logger, string connectionId)
509513
_stoppingClient(logger, DateTime.Now, connectionId, null);
510514
}
511515
}
516+
517+
public static void ExceptionThrownFromEventHandler(this ILogger logger, string connectionId, string eventHandlerName, Exception exception)
518+
{
519+
if (logger.IsEnabled(LogLevel.Error))
520+
{
521+
_exceptionThrownFromHandler(logger, DateTime.Now, connectionId, eventHandlerName, exception);
522+
}
523+
}
512524
}
513525
}

‎test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.cs

Lines changed: 123 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,7 @@ public async Task EventQueueTimeout()
476476
closedTcs.SetResult(null);
477477
return Task.CompletedTask;
478478
};
479-
479+
480480
await connection.StartAsync();
481481
channel.Out.TryWrite(Array.Empty<byte>());
482482

@@ -746,6 +746,128 @@ public async Task CanReceiveData()
746746
}
747747
}
748748

749+
[Fact]
750+
public async Task CanReceiveDataEvenIfUserThrowsInConnectedEvent()
751+
{
752+
var mockHttpHandler = new Mock<HttpMessageHandler>();
753+
mockHttpHandler.Protected()
754+
.Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.IsAny<HttpRequestMessage>(), ItExpr.IsAny<CancellationToken>())
755+
.Returns<HttpRequestMessage, CancellationToken>(async (request, cancellationToken) =>
756+
{
757+
await Task.Yield();
758+
759+
var content = string.Empty;
760+
761+
if (request.Method == HttpMethod.Get)
762+
{
763+
content = "42";
764+
}
765+
766+
return request.Method == HttpMethod.Options
767+
? ResponseUtils.CreateResponse(HttpStatusCode.OK, ResponseUtils.CreateNegotiationResponse())
768+
: ResponseUtils.CreateResponse(HttpStatusCode.OK, content);
769+
});
770+
771+
var connection = new HttpConnection(new Uri("http://fakeuri.org/"), TransportType.LongPolling, loggerFactory: null, httpMessageHandler: mockHttpHandler.Object);
772+
try
773+
{
774+
connection.Connected += () => Task.FromException(new InvalidOperationException());
775+
776+
var receiveTcs = new TaskCompletionSource<string>();
777+
connection.Received += data =>
778+
{
779+
receiveTcs.TrySetResult(Encoding.UTF8.GetString(data));
780+
return Task.CompletedTask;
781+
};
782+
783+
connection.Closed += e =>
784+
{
785+
if (e != null)
786+
{
787+
receiveTcs.TrySetException(e);
788+
}
789+
else
790+
{
791+
receiveTcs.TrySetCanceled();
792+
}
793+
return Task.CompletedTask;
794+
};
795+
796+
await connection.StartAsync();
797+
798+
Assert.Equal("42", await receiveTcs.Task.OrTimeout());
799+
}
800+
finally
801+
{
802+
await connection.DisposeAsync();
803+
}
804+
}
805+
806+
[Fact]
807+
public async Task CanReceiveDataEvenIfExceptionThrownFromPreviousReceivedEvent()
808+
{
809+
var mockHttpHandler = new Mock<HttpMessageHandler>();
810+
mockHttpHandler.Protected()
811+
.Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.IsAny<HttpRequestMessage>(), ItExpr.IsAny<CancellationToken>())
812+
.Returns<HttpRequestMessage, CancellationToken>(async (request, cancellationToken) =>
813+
{
814+
await Task.Yield();
815+
816+
var content = string.Empty;
817+
818+
if (request.Method == HttpMethod.Get)
819+
{
820+
content = "42";
821+
}
822+
823+
return request.Method == HttpMethod.Options
824+
? ResponseUtils.CreateResponse(HttpStatusCode.OK, ResponseUtils.CreateNegotiationResponse())
825+
: ResponseUtils.CreateResponse(HttpStatusCode.OK, content);
826+
});
827+
828+
var connection = new HttpConnection(new Uri("http://fakeuri.org/"), TransportType.LongPolling, loggerFactory: null, httpMessageHandler: mockHttpHandler.Object);
829+
try
830+
{
831+
832+
var receiveTcs = new TaskCompletionSource<string>();
833+
834+
var receivedRaised = false;
835+
connection.Received += data =>
836+
{
837+
if (!receivedRaised)
838+
{
839+
receivedRaised = true;
840+
return Task.FromException(new InvalidOperationException());
841+
}
842+
843+
receiveTcs.TrySetResult(Encoding.UTF8.GetString(data));
844+
return Task.CompletedTask;
845+
};
846+
847+
connection.Closed += e =>
848+
{
849+
if (e != null)
850+
{
851+
receiveTcs.TrySetException(e);
852+
}
853+
else
854+
{
855+
receiveTcs.TrySetCanceled();
856+
}
857+
return Task.CompletedTask;
858+
};
859+
860+
await connection.StartAsync();
861+
862+
Assert.Equal("42", await receiveTcs.Task.OrTimeout());
863+
}
864+
finally
865+
{
866+
await connection.DisposeAsync();
867+
}
868+
}
869+
870+
749871
[Fact]
750872
public async Task CannotSendAfterReceiveThrewException()
751873
{

0 commit comments

Comments
 (0)