Skip to content

Commit 8c4e105

Browse files
olegsycholegsych
authored
Cleanup ExceptionConversionHandlers (#400)
To improve readability of the code, make type names more descriptive: - Rename the internal `V2.Runtime.ExceptionConversionHandler` to `ExceptionSerializer` - Rename the internal `V2.Client.ExceptionConversionHandler` to `ExceptionDeserializer` In types that were already affected by this change, perform additional opportunistic cleanup using primarily Visual Studio tooling and without changing the structure of the code. To simplify code analysis, lock down internal types: - Make internal types sealed, where possible - Make unused members of internal types private - Make fields `readonly` where possible - Use `IEnumerable<>` instead of `List<>` to reduce the risk of unintended modification To improve readability - Use explicit type where variable type doesn't appear explicitly in the initialization statement - Move using directives above namespace, consistently with the .NET coding style instead of inside the namespace style used with C++ - Remove generated comments that duplicate information already available in the code
1 parent 6c3ccc7 commit 8c4e105

File tree

13 files changed

+327
-441
lines changed

13 files changed

+327
-441
lines changed

Diff for: ‎src/Microsoft.ServiceFabric.Services.Remoting/V2/Client/ExceptionConversionHandler.cs renamed to ‎src/Microsoft.ServiceFabric.Services.Remoting/V2/Client/ExceptionDeserializer.cs

+21-48
Original file line numberDiff line numberDiff line change
@@ -11,58 +11,47 @@
1111
using System.Threading.Tasks;
1212
using System.Xml;
1313
using Microsoft.ServiceFabric.Services.Communication;
14-
using Microsoft.ServiceFabric.Services.Remoting.FabricTransport;
1514

1615
namespace Microsoft.ServiceFabric.Services.Remoting.V2.Client
1716
{
18-
internal class ExceptionConversionHandler
17+
sealed class ExceptionDeserializer
1918
{
20-
private static readonly string TraceEventType = "ExceptionConversionHandler";
21-
private IEnumerable<IExceptionConvertor> convertors;
22-
private FabricTransportRemotingSettings remotingSettings;
19+
static readonly string TraceEventType = "ExceptionDeserializer";
20+
readonly IEnumerable<IExceptionConvertor> convertors;
2321

24-
public ExceptionConversionHandler(IEnumerable<IExceptionConvertor> convertors, FabricTransportRemotingSettings remotingSettings)
22+
public ExceptionDeserializer(IEnumerable<IExceptionConvertor> convertors)
2523
{
2624
this.convertors = convertors;
27-
this.remotingSettings = remotingSettings;
2825
}
2926

30-
public Exception FromServiceException(ServiceException serviceException)
27+
Exception FromServiceException(ServiceException serviceException)
3128
{
32-
List<Exception> innerExceptions = new List<Exception>();
29+
var innerExceptions = new List<Exception>();
3330
if (serviceException.ActualInnerExceptions != null && serviceException.ActualInnerExceptions.Count > 0)
3431
{
35-
foreach (var inner in serviceException.ActualInnerExceptions)
36-
{
32+
foreach (ServiceException inner in serviceException.ActualInnerExceptions)
3733
innerExceptions.Add(this.FromServiceException(inner));
38-
}
3934
}
4035

4136
Exception actualEx = null;
42-
foreach (var convertor in this.convertors)
37+
foreach (IExceptionConvertor convertor in this.convertors)
4338
{
4439
try
4540
{
4641
if (innerExceptions.Count == 0)
4742
{
4843
if (convertor.TryConvertFromServiceException(serviceException, out actualEx))
49-
{
5044
break;
51-
}
5245
}
5346
else if (innerExceptions.Count == 1)
5447
{
5548
if (convertor.TryConvertFromServiceException(serviceException, innerExceptions[0], out actualEx))
56-
{
5749
break;
58-
}
5950
}
6051
else
6152
{
6253
if (convertor.TryConvertFromServiceException(serviceException, innerExceptions.ToArray(), out actualEx))
63-
{
6454
break;
65-
}
6655
}
6756
}
6857
catch (Exception ex)
@@ -78,7 +67,7 @@ public Exception FromServiceException(ServiceException serviceException)
7867
return actualEx != null ? actualEx : serviceException;
7968
}
8069

81-
public ServiceException FromRemoteException2(RemoteException2 remoteEx)
70+
ServiceException FromRemoteException2(RemoteException2 remoteEx)
8271
{
8372
var svcEx = new ServiceException(remoteEx.Type, remoteEx.Message);
8473
svcEx.ActualExceptionStackTrace = remoteEx.StackTrace;
@@ -88,29 +77,21 @@ public ServiceException FromRemoteException2(RemoteException2 remoteEx)
8877
{
8978
svcEx.ActualInnerExceptions = new List<ServiceException>();
9079
foreach (var inner in remoteEx.InnerExceptions)
91-
{
9280
svcEx.ActualInnerExceptions.Add(this.FromRemoteException2(inner));
93-
}
9481
}
9582

9683
return svcEx;
9784
}
9885

99-
public RemoteException2 DeserializeRemoteException2(byte[] buffer)
86+
RemoteException2 DeserializeRemoteException2(byte[] buffer)
10087
{
101-
var serializer = new DataContractSerializer(
102-
typeof(RemoteException2),
103-
new DataContractSerializerSettings()
104-
{
105-
MaxItemsInObjectGraph = int.MaxValue,
106-
});
88+
var settings = new DataContractSerializerSettings { MaxItemsInObjectGraph = int.MaxValue };
89+
var serializer = new DataContractSerializer(typeof(RemoteException2), settings);
10790

10891
try
10992
{
110-
using (var reader = XmlDictionaryReader.CreateBinaryReader(buffer, XmlDictionaryReaderQuotas.Max))
111-
{
112-
return (RemoteException2)serializer.ReadObject(reader);
113-
}
93+
using var reader = XmlDictionaryReader.CreateBinaryReader(buffer, XmlDictionaryReaderQuotas.Max);
94+
return (RemoteException2)serializer.ReadObject(reader);
11495
}
11596
catch (Exception e)
11697
{
@@ -125,34 +106,26 @@ public RemoteException2 DeserializeRemoteException2(byte[] buffer)
125106

126107
public async Task DeserializeRemoteExceptionAndThrowAsync(Stream stream)
127108
{
128-
Exception exceptionToThrow = null;
109+
Exception exceptionToThrow;
129110

130111
// Workaround as NativeMessageStream doesn't suport multi read.
131-
var streamLength = stream.Length;
112+
long streamLength = stream.Length;
132113
var buffer = new byte[streamLength];
133114
await stream.ReadAsync(buffer, 0, buffer.Length);
134115
try
135116
{
136-
var remoteException2 = this.DeserializeRemoteException2(buffer);
137-
var svcEx = this.FromRemoteException2(remoteException2);
138-
var ex = this.FromServiceException(svcEx);
139-
140-
if (ex is AggregateException)
141-
{
142-
exceptionToThrow = ex;
143-
}
144-
else
145-
{
146-
exceptionToThrow = new AggregateException(ex);
147-
}
117+
RemoteException2 remoteException2 = this.DeserializeRemoteException2(buffer);
118+
ServiceException svcEx = this.FromRemoteException2(remoteException2);
119+
Exception ex = this.FromServiceException(svcEx);
120+
exceptionToThrow = ex is AggregateException ? ex : new AggregateException(ex);
148121
}
149122
catch (Exception dcsE)
150123
{
151124
exceptionToThrow = new ServiceException(dcsE.GetType().FullName,
152125
string.Format(CultureInfo.InvariantCulture, SR.ErrorDeserializationFailure, dcsE.ToString()));
153126
}
154127

155-
var requestId = LogContext.GetRequestIdOrDefault();
128+
Guid requestId = LogContext.GetRequestIdOrDefault();
156129
ServiceTrace.Source.WriteInfo(
157130
TraceEventType,
158131
"[{0}] Remoting call failed with exception : {1}",

Diff for: ‎src/Microsoft.ServiceFabric.Services.Remoting/V2/FabricTransport/Client/DummyFabricTransportRemotingClient.cs

+6-12
Original file line numberDiff line numberDiff line change
@@ -3,24 +3,18 @@
33
// Licensed under the MIT License (MIT). See License.txt in the repo root for license information.
44
// ------------------------------------------------------------
55

6+
using System;
7+
using System.Threading.Tasks;
8+
using Microsoft.ServiceFabric.FabricTransport.V2.Client;
9+
610
namespace Microsoft.ServiceFabric.Services.Remoting.V2.FabricTransport.Client
711
{
8-
using System;
9-
using System.Threading.Tasks;
10-
using Microsoft.ServiceFabric.FabricTransport.V2.Client;
11-
using Microsoft.ServiceFabric.Services.Remoting.FabricTransport;
12-
using Microsoft.ServiceFabric.Services.Remoting.V2;
13-
14-
internal class DummyFabricTransportRemotingClient : FabricTransportServiceRemotingClient
12+
sealed class DummyFabricTransportRemotingClient : FabricTransportServiceRemotingClient
1513
{
1614
public DummyFabricTransportRemotingClient(
1715
ServiceRemotingMessageSerializersManager serializersManager,
1816
FabricTransportClient fabricTransportClient)
19-
: base(
20-
serializersManager,
21-
fabricTransportClient,
22-
null,
23-
FabricTransportRemotingSettings.GetDefault())
17+
: base(serializersManager, fabricTransportClient, null)
2418
{
2519
}
2620

Diff for: ‎src/Microsoft.ServiceFabric.Services.Remoting/V2/FabricTransport/Client/FabricTransportServiceRemotingClient.cs

+14-16
Original file line numberDiff line numberDiff line change
@@ -3,39 +3,37 @@
33
// Licensed under the MIT License (MIT). See License.txt in the repo root for license information.
44
// ------------------------------------------------------------
55

6+
using System;
7+
using System.Collections.Generic;
8+
using System.Fabric;
9+
using System.Threading;
10+
using System.Threading.Tasks;
11+
using Microsoft.ServiceFabric.FabricTransport.V2;
12+
using Microsoft.ServiceFabric.FabricTransport.V2.Client;
13+
using Microsoft.ServiceFabric.Services.Remoting.V2.Client;
14+
using Microsoft.ServiceFabric.Services.Remoting.V2.Messaging;
15+
616
namespace Microsoft.ServiceFabric.Services.Remoting.V2.FabricTransport.Client
717
{
8-
using System;
9-
using System.Collections.Generic;
10-
using System.Fabric;
11-
using System.Threading;
12-
using System.Threading.Tasks;
13-
using Microsoft.ServiceFabric.FabricTransport.V2;
14-
using Microsoft.ServiceFabric.FabricTransport.V2.Client;
15-
using Microsoft.ServiceFabric.Services.Remoting.FabricTransport;
16-
using Microsoft.ServiceFabric.Services.Remoting.V2.Client;
17-
using Microsoft.ServiceFabric.Services.Remoting.V2.Messaging;
18-
19-
internal class FabricTransportServiceRemotingClient : IServiceRemotingClient
18+
class FabricTransportServiceRemotingClient : IServiceRemotingClient
2019
{
2120
private readonly ServiceRemotingMessageSerializersManager serializersManager;
2221
private readonly FabricTransportClient fabricTransportClient;
2322
private readonly FabricTransportRemotingClientEventHandler remotingHandler;
2423
private ResolvedServicePartition resolvedServicePartition;
2524
private string listenerName;
2625
private ResolvedServiceEndpoint resolvedServiceEndpoint;
27-
private ExceptionConversionHandler exceptionConversionHandler;
26+
private ExceptionDeserializer exceptionDeserializer;
2827

2928
// we need to pass a cache of the serializers here rather than the known types,
3029
// the serializer cache should be maintained by the factor
3130
internal FabricTransportServiceRemotingClient(
3231
ServiceRemotingMessageSerializersManager serializersManager,
3332
FabricTransportClient fabricTransportClient,
3433
FabricTransportRemotingClientEventHandler remotingHandler,
35-
FabricTransportRemotingSettings remotingSettings,
3634
IEnumerable<IExceptionConvertor> exceptionConvertors = null)
3735
{
38-
this.exceptionConversionHandler = new ExceptionConversionHandler(exceptionConvertors, remotingSettings);
36+
this.exceptionDeserializer = new ExceptionDeserializer(exceptionConvertors);
3937
this.fabricTransportClient = fabricTransportClient;
4038
this.remotingHandler = remotingHandler;
4139
this.serializersManager = serializersManager;
@@ -139,7 +137,7 @@ public async Task<IServiceRemotingResponseMessage> RequestResponseAsync(
139137

140138
if (header != null && header.TryGetHeaderValue("HasRemoteException", out var headerValue))
141139
{
142-
await this.exceptionConversionHandler.DeserializeRemoteExceptionAndThrowAsync(retval.GetBody().GetRecievedStream());
140+
await this.exceptionDeserializer.DeserializeRemoteExceptionAndThrowAsync(retval.GetBody().GetRecievedStream());
143141
}
144142

145143
var responseSerializer = this.serializersManager.GetResponseBodySerializer(interfaceId);

Diff for: ‎src/Microsoft.ServiceFabric.Services.Remoting/V2/FabricTransport/Client/FabricTransportServiceRemotingClientFactoryImpl.cs

-1
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,6 @@ protected override Task<FabricTransportServiceRemotingClient> CreateClientAsync(
8787
this.serializersManager,
8888
nativeClient,
8989
remotingHandler,
90-
this.settings,
9190
this.exceptionConvertors);
9291
remotingHandler.ClientConnected += this.OnFabricTransportClientConnected;
9392
remotingHandler.ClientDisconnected += this.OnFabricTransportClientDisconnected;

Diff for: ‎src/Microsoft.ServiceFabric.Services.Remoting/V2/FabricTransport/Runtime/FabricTransportMessageHandler.cs

+15-15
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,19 @@
33
// Licensed under the MIT License (MIT). See License.txt in the repo root for license information.
44
// ------------------------------------------------------------
55

6+
using System;
7+
using System.Collections.Generic;
8+
using System.Diagnostics;
9+
using System.Threading.Tasks;
10+
using Microsoft.ServiceFabric.FabricTransport.V2;
11+
using Microsoft.ServiceFabric.FabricTransport.V2.Runtime;
12+
using Microsoft.ServiceFabric.Services.Remoting.V2.Diagnostic;
13+
using Microsoft.ServiceFabric.Services.Remoting.V2.Messaging;
14+
using Microsoft.ServiceFabric.Services.Remoting.V2.Runtime;
15+
616
namespace Microsoft.ServiceFabric.Services.Remoting.V2.FabricTransport.Runtime
717
{
8-
using System;
9-
using System.Collections.Generic;
10-
using System.Diagnostics;
11-
using System.Threading.Tasks;
12-
using Microsoft.ServiceFabric.FabricTransport.V2;
13-
using Microsoft.ServiceFabric.FabricTransport.V2.Runtime;
14-
using Microsoft.ServiceFabric.Services.Remoting.V2.Diagnostic;
15-
using Microsoft.ServiceFabric.Services.Remoting.V2.Messaging;
16-
using Microsoft.ServiceFabric.Services.Remoting.V2.Runtime;
17-
18-
internal class FabricTransportMessageHandler : IFabricTransportMessageHandler
18+
sealed class FabricTransportMessageHandler : IFabricTransportMessageHandler
1919
{
2020
private static readonly string TraceType = typeof(FabricTransportMessageHandler).Name;
2121
private readonly IServiceRemotingMessageHandler remotingMessageHandler;
@@ -24,12 +24,12 @@ internal class FabricTransportMessageHandler : IFabricTransportMessageHandler
2424
private readonly long replicaOrInstanceId;
2525
private readonly ServiceRemotingPerformanceCounterProvider serviceRemotingPerformanceCounterProvider;
2626
private IServiceRemotingMessageHeaderSerializer headerSerializer;
27-
private ExceptionConversionHandler exceptionConvertorHandler;
27+
private ExceptionSerializer exceptionSerializer;
2828

2929
public FabricTransportMessageHandler(
3030
IServiceRemotingMessageHandler remotingMessageHandler,
3131
ServiceRemotingMessageSerializersManager serializersManager,
32-
ExceptionConversionHandler exceptionConvertorHandler,
32+
ExceptionSerializer exceptionConvertorHandler,
3333
Guid partitionId,
3434
long replicaOrInstanceId)
3535
{
@@ -41,7 +41,7 @@ public FabricTransportMessageHandler(
4141
this.partitionId,
4242
this.replicaOrInstanceId);
4343
this.headerSerializer = this.serializersManager.GetHeaderSerializer();
44-
this.exceptionConvertorHandler = exceptionConvertorHandler;
44+
this.exceptionSerializer = exceptionConvertorHandler;
4545
}
4646

4747
public async Task<FabricTransportMessage> RequestResponseAsync(
@@ -130,7 +130,7 @@ private FabricTransportMessage CreateFabricTransportExceptionMessage(Exception e
130130
header.AddHeader("HasRemoteException", new byte[0]);
131131
var serializedHeader = this.serializersManager.GetHeaderSerializer().SerializeResponseHeader(header);
132132

133-
var serializedMsg = this.exceptionConvertorHandler.SerializeRemoteException(ex);
133+
var serializedMsg = this.exceptionSerializer.SerializeRemoteException(ex);
134134
var msg = new FabricTransportMessage(
135135
new FabricTransportRequestHeader(serializedHeader.GetSendBuffer(), serializedHeader.Dispose),
136136
new FabricTransportRequestBody(serializedMsg, null));

Diff for: ‎src/Microsoft.ServiceFabric.Services.Remoting/V2/FabricTransport/Runtime/FabricTransportServiceRemotingListener.cs

+5-5
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
namespace Microsoft.ServiceFabric.Services.Remoting.V2.FabricTransport.Runtime
1818
{
1919
/// <summary>
20-
/// An <see cref="Microsoft.ServiceFabric.Services.Remoting.Runtime.IServiceRemotingListener"/> that uses
20+
/// An <see cref="IServiceRemotingListener"/> that uses
2121
/// fabric TCP transport to provide interface remoting for stateless and stateful services.
2222
/// </summary>
2323
public class FabricTransportServiceRemotingListener : IServiceRemotingListener
@@ -125,12 +125,12 @@ internal FabricTransportServiceRemotingListener(
125125

126126
svcExceptionConvertors.Add(new FabricExceptionConvertor());
127127
svcExceptionConvertors.Add(new SystemExceptionConvertor());
128-
svcExceptionConvertors.Add(new ExceptionConversionHandler.DefaultExceptionConvertor());
128+
svcExceptionConvertors.Add(new DefaultExceptionConvertor());
129129

130130
this.transportMessageHandler = new FabricTransportMessageHandler(
131131
serviceRemotingMessageHandler,
132132
serializersManager,
133-
new ExceptionConversionHandler(svcExceptionConvertors, remotingSettings),
133+
new ExceptionSerializer(svcExceptionConvertors, remotingSettings),
134134
serviceContext.PartitionId,
135135
serviceContext.ReplicaOrInstanceId);
136136

@@ -151,7 +151,7 @@ internal FabricTransportServiceRemotingListener(
151151
/// </summary>
152152
/// <param name="cancellationToken">Cancellation token</param>
153153
/// <returns>
154-
/// A <see cref="System.Threading.Tasks.Task">Task</see> that represents outstanding operation. The result of the Task is
154+
/// A <see cref="Task"/> that represents outstanding operation. The result of the Task is
155155
/// the endpoint string.
156156
/// </returns>
157157
public Task<string> OpenAsync(CancellationToken cancellationToken)
@@ -179,7 +179,7 @@ public Task<string> OpenAsync(CancellationToken cancellationToken)
179179
/// </summary>
180180
/// <param name="cancellationToken">Cancellation token</param>
181181
/// <returns>
182-
/// A <see cref="System.Threading.Tasks.Task">Task</see> that represents outstanding operation.
182+
/// A <see cref="Task"/> that represents outstanding operation.
183183
/// </returns>
184184
public async Task CloseAsync(CancellationToken cancellationToken)
185185
{

Diff for: ‎src/Microsoft.ServiceFabric.Services.Remoting/V2/Runtime/DefaultExceptionConvertor.cs

+35
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// ------------------------------------------------------------
2+
// Copyright (c) Microsoft Corporation. All rights reserved.
3+
// Licensed under the MIT License (MIT). See License.txt in the repo root for license information.
4+
// ------------------------------------------------------------
5+
6+
using System;
7+
using System.Collections.Generic;
8+
using System.Fabric;
9+
using Microsoft.ServiceFabric.Services.Communication;
10+
11+
namespace Microsoft.ServiceFabric.Services.Remoting.V2.Runtime
12+
{
13+
sealed class DefaultExceptionConvertor : IExceptionConvertor
14+
{
15+
public Exception[] GetInnerExceptions(Exception exception)
16+
{
17+
return exception.InnerException == null ? null : new Exception[] { exception.InnerException };
18+
}
19+
20+
public bool TryConvertToServiceException(Exception originalException, out ServiceException serviceException)
21+
{
22+
serviceException = new ServiceException(originalException.GetType().FullName, originalException.Message);
23+
serviceException.ActualExceptionStackTrace = originalException.StackTrace;
24+
serviceException.ActualExceptionData = new Dictionary<string, string>()
25+
{
26+
{ "HResult", originalException.HResult.ToString() },
27+
};
28+
29+
if (originalException is FabricException fabricEx)
30+
serviceException.ActualExceptionData.Add("FabricErrorCode", ((long)fabricEx.ErrorCode).ToString());
31+
32+
return true;
33+
}
34+
}
35+
}

0 commit comments

Comments
 (0)