Skip to content

Commit 953d65e

Browse files
lilla28lilla28
committed
fix(fdc3-client) - Improve thread safety and error handling in DesktopAgent
- Add semaphore lock to DesktopAgentClient for safe channel access - Ensure initialization before channel operations - Clarify error messages; introduce new error codes (NoInstanceFound, NoAppIntent) - Add ThrowHelper method for backend error reporting - Use Interlocked.Increment for intent message IDs in IntentsClient - Fix spelling/grammar in comments and errors - Remove unnecessary semaphore in ContextListener - Standardize instance ID usage in PrivateChannel
1 parent df64579 commit 953d65e

File tree

14 files changed

+77
-83
lines changed

14 files changed

+77
-83
lines changed

‎src/fdc3/dotnet/DesktopAgent.Client/src/MorganStanley.ComposeUI.Fdc3.DesktopAgent.Client/Infrastructure/DesktopAgentClient.cs‎

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ public class DesktopAgentClient : IDesktopAgent
5050

5151
private readonly ConcurrentDictionary<string, IChannel> _userChannels = new();
5252
private readonly ConcurrentDictionary<string, IChannel> _appChannels = new();
53+
private readonly SemaphoreSlim _appChannelsLock = new(1, 1);
5354

5455
private readonly TaskCompletionSource<string> _initializationTaskCompletionSource = new(TaskCreationOptions.RunContinuationsAsynchronously);
5556
private string? _openedAppContextId;
@@ -313,24 +314,32 @@ public async Task<IImplementationMetadata> GetInfo()
313314
/// <returns></returns>
314315
public async Task<IChannel> GetOrCreateChannel(string channelId)
315316
{
316-
await _initializationTaskCompletionSource.Task.ConfigureAwait(false);
317-
318-
if (_appChannels.TryGetValue(channelId, out var existingChannel))
317+
try
319318
{
320-
return existingChannel;
321-
}
319+
await _initializationTaskCompletionSource.Task.ConfigureAwait(false);
320+
await _appChannelsLock.WaitAsync().ConfigureAwait(false);
322321

323-
var channel = await _channelFactory.CreateAppChannelAsync(channelId);
322+
if (_appChannels.TryGetValue(channelId, out var existingChannel))
323+
{
324+
return existingChannel;
325+
}
324326

325-
if (!_appChannels.TryAdd(channelId, channel))
326-
{
327-
if (_logger.IsEnabled(LogLevel.Warning))
327+
var channel = await _channelFactory.CreateAppChannelAsync(channelId);
328+
329+
if (!_appChannels.TryAdd(channelId, channel))
328330
{
329-
_logger.LogWarning("Failed to add app channel to the internal collection: {ChannelId}.", channelId);
331+
if (_logger.IsEnabled(LogLevel.Warning))
332+
{
333+
_logger.LogWarning("Failed to add app channel to the internal collection: {ChannelId}.", channelId);
334+
}
330335
}
331-
}
332336

333-
return channel;
337+
return channel;
338+
}
339+
finally
340+
{
341+
_appChannelsLock.Release();
342+
}
334343
}
335344

336345
/// <summary>

‎src/fdc3/dotnet/DesktopAgent.Client/src/MorganStanley.ComposeUI.Fdc3.DesktopAgent.Client/Infrastructure/IIntentsClient.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ internal interface IIntentsClient
4242
public ValueTask<IEnumerable<IAppIntent>> FindIntentsByContextAsync(IContext context, string? resultType = null);
4343

4444
/// <summary>
45-
/// Raises an intent for a given context, optionally targeting a specific application. If multiple application can handle the request the container should allow the user to choose the right app.
45+
/// Raises an intent for a given context, optionally targeting a specific application. If multiple applications can handle the request the container should allow the user to choose the right app.
4646
/// </summary>
4747
/// <param name="context"></param>
4848
/// <param name="app"></param>
@@ -61,7 +61,7 @@ public ValueTask<IListener> AddIntentListenerAsync<T>(string intent, IntentHandl
6161
where T : IContext;
6262

6363
/// <summary>
64-
/// Raises an intent, optionally targeting a specific application. If multiple application can handle the request the container should allow the user to choose the right app.
64+
/// Raises an intent, optionally targeting a specific application. If multiple applications can handle the request the container should allow the user to choose the right app.
6565
/// </summary>
6666
/// <param name="intent"></param>
6767
/// <param name="context"></param>

‎src/fdc3/dotnet/DesktopAgent.Client/src/MorganStanley.ComposeUI.Fdc3.DesktopAgent.Client/Infrastructure/Internal/ChannelFactory.cs‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -138,9 +138,9 @@ public async ValueTask<IEnumerable<IChannel>> GetUserChannelsAsync()
138138
throw ThrowHelper.ErrorResponseReceived(response.Error);
139139
}
140140

141-
if (response.Channels == null)
141+
if (response.Channels == null || !response.Channels.Any())
142142
{
143-
throw ThrowHelper.NoChannelsReturned();
143+
throw ThrowHelper.DesktopAgentBackendDidNotResolveRequest(nameof(GetUserChannelsRequest), nameof(response.Channels), Fdc3DesktopAgentErrors.NoUserChannelSetFound);
144144
}
145145

146146
var channels = new List<IChannel>();
@@ -169,11 +169,6 @@ public async ValueTask<IEnumerable<IChannel>> GetUserChannelsAsync()
169169
channels.Add(userChannel);
170170
}
171171

172-
if (!channels.Any())
173-
{
174-
throw ThrowHelper.NoChannelsReturned();
175-
}
176-
177172
return channels;
178173
}
179174

‎src/fdc3/dotnet/DesktopAgent.Client/src/MorganStanley.ComposeUI.Fdc3.DesktopAgent.Client/Infrastructure/Internal/ContextListener.cs‎

Lines changed: 9 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ internal class ContextListener<T> : IListener, IAsyncDisposable
4242
private IAsyncDisposable? _subscription;
4343
private string _contextListenerId;
4444

45-
private readonly SemaphoreSlim _openedAppContextLock = new(1,1);
4645
private bool _isOpenedAppContextHandled = false;
4746

4847
private readonly ConcurrentQueue<T> _contexts = new();
@@ -121,7 +120,6 @@ public async ValueTask SubscribeAsync(string channelId, ChannelType channelType,
121120
try
122121
{
123122
await _subscriptionLock.WaitAsync().ConfigureAwait(false);
124-
await _openedAppContextLock.WaitAsync().ConfigureAwait(false);
125123

126124
if (_isSubscribed)
127125
{
@@ -183,7 +181,6 @@ public async ValueTask SubscribeAsync(string channelId, ChannelType channelType,
183181
finally
184182
{
185183
_subscriptionLock.Release();
186-
_openedAppContextLock.Release();
187184

188185
_logger.LogInformation("Context listener subscribed to channel {ChannelId} for context type {ContextType}.", channelId, _contextType);
189186
}
@@ -194,7 +191,6 @@ public async Task HandleContextAsync(IContext context)
194191
try
195192
{
196193
await _subscriptionLock.WaitAsync().ConfigureAwait(false);
197-
await _openedAppContextLock.WaitAsync().ConfigureAwait(false);
198194

199195
if (!_isSubscribed)
200196
{
@@ -224,42 +220,27 @@ public async Task HandleContextAsync(IContext context)
224220
finally
225221
{
226222
_subscriptionLock.Release();
227-
_openedAppContextLock.Release();
228223
}
229224
}
230225

231-
internal async Task SetOpenHandledAsync(bool handled)
226+
internal Task SetOpenHandledAsync(bool handled)
232227
{
233-
try
234-
{
235-
await _openedAppContextLock.WaitAsync().ConfigureAwait(false);
236-
_isOpenedAppContextHandled = handled;
237-
}
238-
finally
239-
{
240-
_openedAppContextLock.Release();
241-
}
228+
_isOpenedAppContextHandled = handled;
229+
return Task.CompletedTask;
242230
}
243231

244232
//TODO: decide if we want to handle the cached contexts after the context via the fdc3.open call is handled
245-
private async Task HandleCachedContextsAsync()
233+
private Task HandleCachedContextsAsync()
246234
{
247-
try
235+
if (_isOpenedAppContextHandled)
248236
{
249-
await _openedAppContextLock.WaitAsync().ConfigureAwait(false);
250-
251-
if (_isOpenedAppContextHandled)
237+
while (_contexts.TryDequeue(out var context))
252238
{
253-
while (_contexts.TryDequeue(out var context))
254-
{
255-
_contextHandler(context);
256-
}
239+
_contextHandler(context);
257240
}
258241
}
259-
finally
260-
{
261-
_openedAppContextLock.Release();
262-
}
242+
243+
return Task.CompletedTask;
263244
}
264245

265246
private async ValueTask RegisterContextListenerAsync(

‎src/fdc3/dotnet/DesktopAgent.Client/src/MorganStanley.ComposeUI.Fdc3.DesktopAgent.Client/Infrastructure/Internal/IntentsClient.cs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
* and limitations under the License.
1313
*/
1414

15-
using System.Security.Cryptography;
1615
using System.Text.Json;
1716
using Finos.Fdc3;
1817
using Finos.Fdc3.Context;
@@ -29,6 +28,8 @@ namespace MorganStanley.ComposeUI.Fdc3.DesktopAgent.Client.Infrastructure.Intern
2928

3029
internal class IntentsClient : IIntentsClient
3130
{
31+
private static int _messageIdCounter = 0;
32+
3233
private readonly IMessaging _messaging;
3334
private readonly IChannelFactory _channelFactory;
3435
private readonly string _instanceId;
@@ -127,7 +128,7 @@ public async ValueTask<IAppIntent> FindIntentAsync(string intent, IContext? cont
127128

128129
if (response.AppIntent == null)
129130
{
130-
throw ThrowHelper.AppIntentMissingFromResponse(intent, context?.Type, resultType);
131+
throw ThrowHelper.DesktopAgentBackendDidNotResolveRequest(nameof(FindIntentRequest), nameof(response.AppIntent), Fdc3DesktopAgentErrors.NoAppIntent);
131132
}
132133

133134
return response.AppIntent;
@@ -172,7 +173,7 @@ public async ValueTask<IEnumerable<IAppIntent>> FindIntentsByContextAsync(IConte
172173

173174
public async ValueTask<IIntentResolution> RaiseIntentAsync(string intent, IContext context, IAppIdentifier? app)
174175
{
175-
var messageId = Guid.NewGuid().GetHashCode();
176+
var messageId = Interlocked.Increment(ref _messageIdCounter);
176177

177178
var request = new RaiseIntentRequest
178179
{
@@ -231,7 +232,7 @@ public async ValueTask<IIntentResolution> RaiseIntentAsync(string intent, IConte
231232

232233
public async ValueTask<IIntentResolution> RaiseIntentForContextAsync(IContext context, IAppIdentifier? app)
233234
{
234-
var messageId = Guid.NewGuid().GetHashCode();
235+
var messageId = Interlocked.Increment(ref _messageIdCounter);
235236

236237
var request = new RaiseIntentForContextRequest
237238
{

‎src/fdc3/dotnet/DesktopAgent.Client/src/MorganStanley.ComposeUI.Fdc3.DesktopAgent.Client/Infrastructure/Internal/MetadataClient.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ public async ValueTask<IEnumerable<IAppIdentifier>> FindInstancesAsync(IAppIdent
7373

7474
if (response.Instances == null)
7575
{
76-
throw ThrowHelper.ErrorResponseReceived(Fdc3DesktopAgentErrors.UnspecifiedReason);
76+
throw ThrowHelper.DesktopAgentBackendDidNotResolveRequest(nameof(FindInstancesRequest), nameof(response.Instances), Fdc3DesktopAgentErrors.NoInstanceFound);
7777
}
7878

7979
return response.Instances;

‎src/fdc3/dotnet/DesktopAgent.Client/src/MorganStanley.ComposeUI.Fdc3.DesktopAgent.Client/Infrastructure/Internal/PrivateChannelDisconnectEventListener.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,19 @@
1919
namespace MorganStanley.ComposeUI.Fdc3.DesktopAgent.Client.Infrastructure.Internal;
2020

2121
internal delegate void PrivateChannelDisconnectEventHandler();
22-
internal delegate void PrivateChannelOnDisconnectHandler(PrivateChannelDisconnectEventListener listener);
22+
internal delegate void PrivateChannelUnsubscribeHandler(PrivateChannelDisconnectEventListener listener);
2323

2424
internal class PrivateChannelDisconnectEventListener : IListener
2525
{
2626
private readonly PrivateChannelDisconnectEventHandler _onDisconnect;
27-
private readonly PrivateChannelOnDisconnectHandler _onUnsubscribe;
27+
private readonly PrivateChannelUnsubscribeHandler _onUnsubscribe;
2828
private readonly ILogger<PrivateChannelDisconnectEventListener> _logger;
2929
private readonly SemaphoreSlim _semaphoreSlim = new(1, 1);
3030
private bool _subscribed;
3131

3232
public PrivateChannelDisconnectEventListener(
3333
PrivateChannelDisconnectEventHandler onDisconnectEventHandler,
34-
PrivateChannelOnDisconnectHandler onUnsubscribeHandler,
34+
PrivateChannelUnsubscribeHandler onUnsubscribeHandler,
3535
ILogger<PrivateChannelDisconnectEventListener>? logger = null)
3636
{
3737
_onDisconnect = onDisconnectEventHandler;

‎src/fdc3/dotnet/DesktopAgent.Client/src/MorganStanley.ComposeUI.Fdc3.DesktopAgent.Client/Infrastructure/Internal/Protocol/Channel.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ public Channel(
6868

6969
protected ILoggerFactory LoggerFactory => _loggerFactory;
7070
protected IMessaging Messaging => _messaging;
71+
protected string InstanceId => _instanceId;
7172

7273
public virtual async Task<IListener> AddContextListener<T>(string? contextType, ContextHandler<T> handler) where T : IContext
7374
{

0 commit comments

Comments
 (0)