Skip to content

Commit 6839f9f

Browse files
shjellvikshjellvik
committed
fix: make OsduClient service-client initialisation thread-safe
1 parent c34d16b commit 6839f9f

2 files changed

Lines changed: 226 additions & 56 deletions

File tree

‎src/OsduCsharpClient/Facade/OsduClient.cs‎

Lines changed: 73 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -41,30 +41,22 @@ public sealed class OsduClient : IDisposable
4141
private readonly OsduConfig _config;
4242
private readonly ITokenProvider _tokenProvider;
4343
private readonly ILoggerFactory _loggerFactory;
44+
45+
/// <summary>
46+
/// Guards all lazy initialisation state below. Service clients and their adapters are
47+
/// built on first access, so concurrent first calls would otherwise race on the
48+
/// dictionaries and on <see cref="_httpClients"/>. Contention is negligible: each
49+
/// service is built at most once per client instance.
50+
/// </summary>
51+
private readonly Lock _sync = new();
52+
4453
private readonly List<HttpClient> _httpClients = [];
4554
private readonly Dictionary<string, HttpClientRequestAdapter> _adapters = [];
46-
private bool _disposed;
4755

48-
private CrsCatalogClient? _crsCatalog;
49-
private CrsConversionClient? _crsConversion;
50-
private DatasetClient? _dataset;
51-
private EntitlementsClient? _entitlements;
52-
private FileClient? _file;
53-
private GeospatialClient? _geospatial;
54-
private IndexerClient? _indexer;
55-
private LegalClient? _legal;
56-
private NotificationClient? _notification;
57-
private PartitionClient? _partition;
58-
private PolicyClient? _policy;
59-
private RegisterClient? _register;
60-
private SchemaClient? _schema;
61-
private SearchClient? _search;
62-
private SeismicDdmsClient? _seismicDdms;
63-
private StorageClient? _storage;
64-
private UnitClient? _unit;
65-
private WellboreDdmsClient? _wellboreDdms;
66-
private WorkflowClient? _workflow;
67-
private WellboreDdmsBulkClient? _wellboreDdmsBulk;
56+
/// <summary>Built service clients, keyed by client type. Guarded by <see cref="_sync"/>.</summary>
57+
private readonly Dictionary<Type, object> _clients = [];
58+
59+
private bool _disposed;
6860

6961
/// <param name="config">OSDU configuration. Use <see cref="OsduConfig.FromConfiguration"/> to bind from <c>IConfiguration</c>.</param>
7062
/// <param name="tokenProvider">
@@ -83,34 +75,33 @@ public OsduClient(OsduConfig config, ITokenProvider? tokenProvider = null, ILogg
8375
_loggerFactory = loggerFactory ?? NullLoggerFactory.Instance;
8476
}
8577

86-
public CrsCatalogClient CrsCatalog => _crsCatalog ??= Build(ref _crsCatalog, "crs_catalog");
87-
public CrsConversionClient CrsConversion => _crsConversion ??= Build(ref _crsConversion, "crs_conversion");
88-
public DatasetClient Dataset => _dataset ??= Build(ref _dataset, "dataset");
89-
public EntitlementsClient Entitlements => _entitlements ??= Build(ref _entitlements, "entitlements");
90-
public FileClient File => _file ??= Build(ref _file, "file");
91-
public GeospatialClient Geospatial => _geospatial ??= Build(ref _geospatial, "geospatial");
92-
public IndexerClient Indexer => _indexer ??= Build(ref _indexer, "indexer");
93-
public LegalClient Legal => _legal ??= Build(ref _legal, "legal");
94-
public NotificationClient Notification => _notification ??= Build(ref _notification, "notification");
95-
public PartitionClient Partition => _partition ??= Build(ref _partition, "partition");
96-
public PolicyClient Policy => _policy ??= Build(ref _policy, "policy");
97-
public RegisterClient Register => _register ??= Build(ref _register, "register");
98-
public SchemaClient Schema => _schema ??= Build(ref _schema, "schema");
99-
public SearchClient Search => _search ??= Build(ref _search, "search");
100-
public SeismicDdmsClient SeismicDdms => _seismicDdms ??= Build(ref _seismicDdms, "seismic_ddms");
101-
public StorageClient Storage => _storage ??= Build(ref _storage, "storage");
102-
public UnitClient Unit => _unit ??= Build(ref _unit, "unit");
103-
public WellboreDdmsClient WellboreDdms => _wellboreDdms ??= Build(ref _wellboreDdms, "wellbore_ddms");
104-
public WorkflowClient Workflow => _workflow ??= Build(ref _workflow, "workflow");
78+
public CrsCatalogClient CrsCatalog => Client<CrsCatalogClient>("crs_catalog");
79+
public CrsConversionClient CrsConversion => Client<CrsConversionClient>("crs_conversion");
80+
public DatasetClient Dataset => Client<DatasetClient>("dataset");
81+
public EntitlementsClient Entitlements => Client<EntitlementsClient>("entitlements");
82+
public FileClient File => Client<FileClient>("file");
83+
public GeospatialClient Geospatial => Client<GeospatialClient>("geospatial");
84+
public IndexerClient Indexer => Client<IndexerClient>("indexer");
85+
public LegalClient Legal => Client<LegalClient>("legal");
86+
public NotificationClient Notification => Client<NotificationClient>("notification");
87+
public PartitionClient Partition => Client<PartitionClient>("partition");
88+
public PolicyClient Policy => Client<PolicyClient>("policy");
89+
public RegisterClient Register => Client<RegisterClient>("register");
90+
public SchemaClient Schema => Client<SchemaClient>("schema");
91+
public SearchClient Search => Client<SearchClient>("search");
92+
public SeismicDdmsClient SeismicDdms => Client<SeismicDdmsClient>("seismic_ddms");
93+
public StorageClient Storage => Client<StorageClient>("storage");
94+
public UnitClient Unit => Client<UnitClient>("unit");
95+
public WellboreDdmsClient WellboreDdms => Client<WellboreDdmsClient>("wellbore_ddms");
96+
public WorkflowClient Workflow => Client<WorkflowClient>("workflow");
10597

10698
/// <summary>
10799
/// Hand-written Wellbore DDMS bulk-data helpers for <c>application/x-parquet</c>
108100
/// (read, write, and chunked session writes), which the generated
109101
/// <see cref="WellboreDdms"/> client cannot express. Shares the same
110102
/// authenticated transport as <see cref="WellboreDdms"/>.
111103
/// </summary>
112-
public WellboreDdmsBulkClient WellboreDdmsBulk =>
113-
_wellboreDdmsBulk ??= new WellboreDdmsBulkClient(GetOrCreateAdapter("wellbore_ddms"));
104+
public WellboreDdmsBulkClient WellboreDdmsBulk => Client<WellboreDdmsBulkClient>("wellbore_ddms");
114105

115106
/// <summary>
116107
/// Returns the authenticated Kiota request adapter for the given service attr name
@@ -121,20 +112,39 @@ public OsduClient(OsduConfig config, ITokenProvider? tokenProvider = null, ILogg
121112
/// </summary>
122113
public IRequestAdapter GetRequestAdapter(string serviceAttr) => GetOrCreateAdapter(serviceAttr);
123114

124-
private T Build<T>(ref T? field, string serviceAttr) where T : class
115+
/// <summary>
116+
/// Returns the cached service client of type <typeparamref name="T"/>, building it
117+
/// (and its adapter) on first access. Keyed by client type rather than by
118+
/// <paramref name="serviceAttr"/> so that two client types may share one adapter —
119+
/// <see cref="WellboreDdms"/> and <see cref="WellboreDdmsBulk"/> both use
120+
/// <c>wellbore_ddms</c>.
121+
/// </summary>
122+
private T Client<T>(string serviceAttr) where T : class
125123
{
126-
if (field is not null) return field;
127-
var adapter = GetOrCreateAdapter(serviceAttr);
128-
field = (T)Activator.CreateInstance(typeof(T), adapter)!;
129-
return field;
124+
lock (_sync)
125+
{
126+
ObjectDisposedException.ThrowIf(_disposed, this);
127+
128+
if (_clients.TryGetValue(typeof(T), out var existing)) return (T)existing;
129+
130+
var client = (T)Activator.CreateInstance(typeof(T), GetOrCreateAdapter(serviceAttr))!;
131+
_clients[typeof(T)] = client;
132+
return client;
133+
}
130134
}
131135

132136
private HttpClientRequestAdapter GetOrCreateAdapter(string serviceAttr)
133137
{
134-
if (_adapters.TryGetValue(serviceAttr, out var adapter)) return adapter;
135-
adapter = CreateAdapter(_config.UrlFor(serviceAttr));
136-
_adapters[serviceAttr] = adapter;
137-
return adapter;
138+
lock (_sync)
139+
{
140+
ObjectDisposedException.ThrowIf(_disposed, this);
141+
142+
if (_adapters.TryGetValue(serviceAttr, out var adapter)) return adapter;
143+
144+
adapter = CreateAdapter(_config.UrlFor(serviceAttr));
145+
_adapters[serviceAttr] = adapter;
146+
return adapter;
147+
}
138148
}
139149

140150
/// <summary>
@@ -168,11 +178,18 @@ private HttpClientRequestAdapter CreateAdapter(string baseUrl)
168178

169179
public void Dispose()
170180
{
171-
if (_disposed) return;
172-
_disposed = true;
173-
foreach (var client in _httpClients)
174-
client.Dispose();
175-
_httpClients.Clear();
181+
lock (_sync)
182+
{
183+
if (_disposed) return;
184+
_disposed = true;
185+
186+
foreach (var client in _httpClients)
187+
client.Dispose();
188+
189+
_httpClients.Clear();
190+
_adapters.Clear();
191+
_clients.Clear();
192+
}
176193
}
177194

178195
/// <summary>Adapts <see cref="ITokenProvider"/> to Kiota's <see cref="IAccessTokenProvider"/>.</summary>

‎tests/OsduCsharpClient.Tests/OsduClientConcurrencyTests.cs‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
using Equinor.OsduCsharpClient.Facade;
2+
using Equinor.OsduCsharpClient.Facade.Auth;
3+
using Equinor.OsduCsharpClient.Search;
4+
using Equinor.OsduCsharpClient.WellboreDdms;
5+
using Xunit;
6+
7+
namespace OsduCsharpClient.Tests;
8+
9+
/// <summary>
10+
/// Service clients and their request adapters are built lazily on first property access.
11+
/// A singleton <see cref="OsduClient"/> (the normal DI registration) is hit by many
12+
/// requests at once during cold start, so that first access must be safe to race.
13+
///
14+
/// Before the lock was introduced these tests failed the great majority of runs: racing
15+
/// threads each built their own client over their own <c>HttpClient</c>, leaving orphaned
16+
/// handlers behind, and the unsynchronised <c>List&lt;HttpClient&gt;</c> could be corrupted
17+
/// badly enough that <see cref="OsduClient.Dispose"/> threw a <see cref="NullReferenceException"/>.
18+
/// </summary>
19+
public class OsduClientConcurrencyTests
20+
{
21+
private const int Threads = 16;
22+
private const int Trials = 50;
23+
24+
private static OsduConfig MakeConfig() => new()
25+
{
26+
Server = "https://osdu.example.com",
27+
DataPartitionId = "test-partition",
28+
Authority = "https://login.microsoftonline.com/tenant",
29+
ClientId = "client-id",
30+
Scopes = "https://example.com/.default",
31+
};
32+
33+
private static OsduClient NewClient() => new(MakeConfig(), new StaticTokenProvider("tok"));
34+
35+
/// <summary>
36+
/// Runs <paramref name="access"/> on <see cref="Threads"/> threads released simultaneously,
37+
/// and returns what each one observed. Exceptions are captured rather than thrown so a
38+
/// failing run reports every thread's outcome at once.
39+
/// </summary>
40+
private static (object?[] Results, Exception?[] Failures) Race(Func<int, object?> access)
41+
{
42+
var results = new object?[Threads];
43+
var failures = new Exception?[Threads];
44+
using var gate = new Barrier(Threads);
45+
var threads = new Thread[Threads];
46+
47+
for (var i = 0; i < Threads; i++)
48+
{
49+
var index = i;
50+
threads[i] = new Thread(() =>
51+
{
52+
gate.SignalAndWait();
53+
try { results[index] = access(index); }
54+
catch (Exception ex) { failures[index] = ex; }
55+
});
56+
threads[i].Start();
57+
}
58+
59+
foreach (var thread in threads) thread.Join();
60+
return (results, failures);
61+
}
62+
63+
[Fact]
64+
public void ServiceProperty_ReturnsSameInstance_WhenFirstAccessIsRaced()
65+
{
66+
for (var trial = 0; trial < Trials; trial++)
67+
{
68+
using var client = NewClient();
69+
70+
var (results, failures) = Race(_ => client.Search);
71+
72+
Assert.All(failures, Assert.Null);
73+
Assert.Single(results.Distinct());
74+
}
75+
}
76+
77+
[Fact]
78+
public void DistinctServiceProperties_InitialiseSafely_WhenRaced()
79+
{
80+
for (var trial = 0; trial < Trials; trial++)
81+
{
82+
var client = NewClient();
83+
84+
// Spread the threads across different services so the adapter dictionary and the
85+
// HttpClient list take concurrent writes for several distinct keys at once.
86+
var (results, failures) = Race(index => (index % 4) switch
87+
{
88+
0 => client.Search,
89+
1 => client.Storage,
90+
2 => client.WellboreDdms,
91+
_ => client.Entitlements,
92+
});
93+
94+
Assert.All(failures, Assert.Null);
95+
96+
// Every thread that asked for a given service must have been handed the same
97+
// instance, even though four services were being built at the same time.
98+
foreach (var perService in results.GroupBy(client => client!.GetType()))
99+
Assert.Single(perService.Distinct());
100+
101+
// Disposal walks the HttpClient list, which is what a torn write corrupts.
102+
var dispose = Record.Exception(client.Dispose);
103+
Assert.Null(dispose);
104+
}
105+
}
106+
107+
[Fact]
108+
public void GetRequestAdapter_ReturnsSameAdapter_WhenFirstAccessIsRaced()
109+
{
110+
for (var trial = 0; trial < Trials; trial++)
111+
{
112+
using var client = NewClient();
113+
114+
var (results, failures) = Race(_ => client.GetRequestAdapter("search"));
115+
116+
Assert.All(failures, Assert.Null);
117+
Assert.Single(results.Distinct());
118+
}
119+
}
120+
121+
[Fact]
122+
public void WellboreDdmsAndBulk_AreDistinctClients_OverOneSharedAdapter()
123+
{
124+
using var client = NewClient();
125+
126+
// Both are served from the "wellbore_ddms" adapter, so the client cache has to be
127+
// keyed by client type rather than by service name.
128+
Assert.IsType<WellboreDdmsClient>(client.WellboreDdms);
129+
Assert.IsType<WellboreDdmsBulkClient>(client.WellboreDdmsBulk);
130+
Assert.Same(client.WellboreDdms, client.WellboreDdms);
131+
Assert.Same(client.WellboreDdmsBulk, client.WellboreDdmsBulk);
132+
}
133+
134+
[Fact]
135+
public void ServiceProperty_ReturnsSameInstance_AcrossSequentialAccesses()
136+
{
137+
using var client = NewClient();
138+
139+
Assert.Same(client.Search, client.Search);
140+
Assert.IsType<SearchClient>(client.Search);
141+
}
142+
143+
[Fact]
144+
public void ServiceProperty_Throws_AfterDispose()
145+
{
146+
var client = NewClient();
147+
_ = client.Search;
148+
client.Dispose();
149+
150+
Assert.Throws<ObjectDisposedException>(() => client.Search);
151+
Assert.Throws<ObjectDisposedException>(() => client.GetRequestAdapter("search"));
152+
}
153+
}

0 commit comments

Comments
 (0)