#770 FlurlClientCache.Add throws if client doesn't exist

GetOrAdd should be used to add clients on demand

src/Flurl.Http/Configuration/FlurlClientCache.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Concurrent;
+using System.Collections.Generic;
 
 namespace Flurl.Http.Configuration
 {
@@ -17,14 +18,14 @@ public interface IFlurlClientCache
 		IFlurlClientBuilder Add(string name, string baseUrl = null);
 
 		/// <summary>
-		/// Gets a named IFlurlClient, creating one if it doesn't exist or has been disposed.
+		/// Gets a preconfigured named IFlurlClient.
 		/// </summary>
 		/// <param name="name">The client name.</param>
 		/// <returns>The cached IFlurlClient.</returns>
 		IFlurlClient Get(string name);
 
 		/// <summary>
-		/// Gets a named IFlurlClient, creating and configuring one if it doesn't exist or has been disposed.
+		/// Gets a named IFlurlClient, creating and (optionally) configuring one if it doesn't exist or has been disposed.
 		/// </summary>
 		/// <param name="name">The client name.</param>
 		/// <param name="baseUrl">The base URL associated with the new client, if it doesn't exist.</param>
@@ -76,38 +77,44 @@ public static IFlurlClientCache Add(this IFlurlClientCache cache, string name, s
 	/// <summary>
 	/// Default implementation of IFlurlClientCache.
 	/// </summary>
-	public class FlurlClientCache : IFlurlClientCache {
+	public class FlurlClientCache : IFlurlClientCache
+	{
 		private readonly ConcurrentDictionary<string, Lazy<IFlurlClient>> _clients = new();
-		private Action<IFlurlClientBuilder> _configureAll;
+		private readonly List<Action<IFlurlClientBuilder>> _configureAll = new();
 
 		/// <inheritdoc />
 		public IFlurlClientBuilder Add(string name, string baseUrl = null) {
 			if (name == null)
 				throw new ArgumentNullException(nameof(name));
 
-			var builder = new FlurlClientBuilder(baseUrl);
-			Lazy<IFlurlClient> Create() {
-				_configureAll?.Invoke(builder);
-				return new Lazy<IFlurlClient>(builder.Build);
-			}
-
-			if (!_clients.TryAdd(name, Create()))
-				throw new ArgumentException($"A client named '{name}' was already registered with this factory. Add should be called just once per client at startup.");
+			var builder = CreateBuilder(baseUrl);
+			if (!_clients.TryAdd(name, new Lazy<IFlurlClient>(builder.Build)))
+				throw new ArgumentException($"A client named '{name}' was already registered. Add should be called just once per client at startup.");
 
 			return builder;
 		}
 
 		/// <inheritdoc />
-		public virtual IFlurlClient Get(string name) => GetOrAdd(name);
+		public virtual IFlurlClient Get(string name) {
+			if (name == null)
+				throw new ArgumentNullException(nameof(name));
+
+			if (!_clients.TryGetValue(name, out var cli))
+				throw new ArgumentException($"A client named '{name}' was not found. Either preconfigure the client using Add (typically at startup), or use GetOrAdd to add/configure one on demand when needed.");
+
+			if (cli.Value.IsDisposed)
+				throw new Exception($"A client named '{name}' was not found but has been disposed and cannot be reused.");
+
+			return cli.Value;
+		}
 
 		/// <inheritdoc />
 		public IFlurlClient GetOrAdd(string name, string baseUrl = null, Action<IFlurlClientBuilder> configure = null) {
 			if (name == null)
 				throw new ArgumentNullException(nameof(name));
 
-			var builder = new FlurlClientBuilder(baseUrl);
 			Lazy<IFlurlClient> Create() {
-				_configureAll?.Invoke(builder);
+				var builder = CreateBuilder(baseUrl);
 				configure?.Invoke(builder);
 				return new Lazy<IFlurlClient>(builder.Build);
 			}
@@ -117,7 +124,8 @@ Lazy<IFlurlClient> Create() {
 
 		/// <inheritdoc />
 		public IFlurlClientCache ConfigureAll(Action<IFlurlClientBuilder> configure) {
-			_configureAll = configure;
+			if (configure != null)
+				_configureAll.Add(configure);
 			return this;
 		}
 
@@ -135,5 +143,12 @@ public IFlurlClientCache Clear() {
 				Remove(key);
 			return this;
 		}
+
+		private IFlurlClientBuilder CreateBuilder(string baseUrl) {
+			var builder = new FlurlClientBuilder(baseUrl);
+			foreach (var config in _configureAll)
+				config(builder);
+			return builder;
+		}
 	}
 }

src/Flurl.Http/FlurlHttp.cs

@@ -26,7 +26,7 @@ public static class FlurlHttp
 		/// <summary>
 		/// Gets or creates the IFlurlClient that would be selected for sending the given IFlurlRequest when the clientless pattern is used.
 		/// </summary>
-		public static IFlurlClient GetClientForRequest(IFlurlRequest req) => Clients.Get(_cachingStrategy(req));
+		public static IFlurlClient GetClientForRequest(IFlurlRequest req) => Clients.GetOrAdd(_cachingStrategy(req));
 
 		/// <summary>
 		/// Sets a global caching strategy for getting or creating an IFlurlClient instance when the clientless pattern is used, e.g. url.GetAsync.

test/Flurl.Test/Http/FlurlClientCacheTests.cs

@@ -44,14 +44,14 @@ public void can_get_or_add_client() {
 		}
 
 		[Test]
-		public void can_get_unconfigured_client() {
+		public void can_get_or_add_unconfigured_client() {
 			var cache = new FlurlClientCache();
-			var cli = cache.Get("foo");
+			var cli = cache.GetOrAdd("foo");
+
 			Assert.IsNull(cli.BaseUrl);
 			Assert.AreEqual(100, cli.Settings.Timeout.Value.TotalSeconds);
-
-			Assert.AreSame(cli, cache.Get("foo"), "should reuse same-named clients.");
-			Assert.AreNotSame(cli, cache.Get("bar"), "different-named clients should be different instances.");
+			Assert.AreSame(cli, cache.GetOrAdd("foo"), "should reuse same-named clients.");
+			Assert.AreNotSame(cli, cache.GetOrAdd("bar"), "different-named clients should be different instances.");
 		}
 
 		[Test]
@@ -66,7 +66,7 @@ public void can_configure_all() {
 			var cache = new FlurlClientCache()
 				.ConfigureAll(b => b.WithSettings(s => s.Timeout = TimeSpan.FromSeconds(123)));
 
-			var cli1 = cache.Get("foo");
+			var cli1 = cache.GetOrAdd("foo");
 
 			cache.Add("bar").WithSettings(s => {
 				s.Timeout = TimeSpan.FromSeconds(456);
@@ -75,8 +75,8 @@ public void can_configure_all() {
 				s.Timeout = TimeSpan.FromSeconds(789);
 			}));
 
-			var cli2 = cache.Get("bar");
-			var cli3 = cache.Get("buzz");
+			var cli2 = cache.GetOrAdd("bar");
+			var cli3 = cache.GetOrAdd("buzz");
 
 			Assert.AreEqual(123, cli1.Settings.Timeout.Value.TotalSeconds, "fetched the client before changing the defaults, so original should stick");
 			Assert.AreEqual(456, cli2.Settings.Timeout.Value.TotalSeconds, "client-specific settings should always win, even if defaults were changed after");
@@ -92,7 +92,9 @@ public void can_remove() {
 			var cli1 = cache.Get("foo");
 			var cli2 = cache.Get("foo");
 			cache.Remove("foo");
-			var cli3 = cache.Get("foo");
+
+			Assert.Throws<ArgumentException>(() => cache.Get("foo"));
+			var cli3 = cache.GetOrAdd("foo");
 
 			Assert.AreSame(cli1, cli2);
 			Assert.AreNotSame(cli1, cli3);