From 39b89c643eb79a1ad7c63ed138c33bd9306fd598 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Fri, 13 Nov 2026 13:46:46 +0000 Subject: [PATCH 1/5] Limit the cloud host lookup to the timeout and log why processing failed The Cloud Request Engine sets its timeout, 2 seconds by default, as the connect and read timeouts of each HttpURLConnection. Neither covers the lookup of the host, which waits as long as the system resolver does, so a resolver that stops answering held a request for 30 seconds. HttpClientDefault now looks up the host within the connection's connect timeout before connecting. Callers waiting on the same host share one lookup, and a request through a proxy is not looked up. The error FlowData logs for a processing failure now carries the exception. The location packages took SLF4J 1.7.36 with Logback 1.3.16, which binds only through the SLF4J 2 API, so their tests logged nothing. They now take SLF4J 2.0.20, as the other packages do. --- location/pom.xml | 2 +- .../core/flowelements/FlowDataDefault.java | 2 +- .../core/flowelements/FlowDataTests.java | 19 ++ .../pipeline/engines/services/HostLookup.java | 198 +++++++++++++ .../engines/services/HttpClientDefault.java | 24 ++ .../engines/services/HostLookupTests.java | 266 ++++++++++++++++++ 6 files changed, 509 insertions(+), 2 deletions(-) create mode 100644 pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HostLookup.java create mode 100644 pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/HostLookupTests.java diff --git a/location/pom.xml b/location/pom.xml index 9974783aa..351582e43 100644 --- a/location/pom.xml +++ b/location/pom.xml @@ -63,7 +63,7 @@ 4.5.130 4.13.2 - 1.7.36 + 2.0.20 1.3.16 4.3.1 20211205 diff --git a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowDataDefault.java b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowDataDefault.java index f8758e126..6b1017929 100644 --- a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowDataDefault.java +++ b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowDataDefault.java @@ -121,7 +121,7 @@ public void addError(FlowError error) { error.getFlowElement().getClass().getSimpleName() + "-" + error.getFlowElement().hashCode(); } - logger.error(message); + logger.error(message, error.getThrowable()); } } diff --git a/pipeline/pipeline.core/src/test/java/fiftyone/pipeline/core/flowelements/FlowDataTests.java b/pipeline/pipeline.core/src/test/java/fiftyone/pipeline/core/flowelements/FlowDataTests.java index 613033fa2..e2235f987 100644 --- a/pipeline/pipeline.core/src/test/java/fiftyone/pipeline/core/flowelements/FlowDataTests.java +++ b/pipeline/pipeline.core/src/test/java/fiftyone/pipeline/core/flowelements/FlowDataTests.java @@ -317,6 +317,25 @@ public void FlowData_GetNotPresent_Element() { assertNull(result); } + /** + * The error logged for a processing failure carries the exception, so + * the log says why processing failed. + */ + @Test + public void FlowData_AddError_LogsTheException() throws Exception { + Logger logger = mock(Logger.class); + when(logger.isErrorEnabled()).thenReturn(true); + Exception cause = new Exception("The cause of the failure."); + try (FlowData data = new FlowDataDefault( + logger, + pipeline, + new EvidenceDefault(mock(Logger.class)))) { + + data.addError(cause, mock(FlowElement.class)); + } + verify(logger).error(anyString(), same(cause)); + } + @Test(expected = IllegalStateException.class) public void FlowData_PipelineDisposed() throws Exception { when(pipeline.isClosed()).thenReturn(true); diff --git a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HostLookup.java b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HostLookup.java new file mode 100644 index 000000000..0fae02581 --- /dev/null +++ b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HostLookup.java @@ -0,0 +1,198 @@ +/* ********************************************************************* + * This Original Work is copyright of 51 Degrees Mobile Experts Limited. + * Copyright 2026 51 Degrees Mobile Experts Limited, Davidson House, + * Forbury Square, Reading, Berkshire, United Kingdom RG1 3EU. + * + * This Original Work is licensed under the European Union Public Licence + * (EUPL) v.1.2 and is subject to its terms as set out below. + * + * If a copy of the EUPL was not distributed with this file, You can obtain + * one at https://opensource.org/licenses/EUPL-1.2. + * + * The 'Compatible Licences' set out in the Appendix to the EUPL (as may be + * amended by the European Commission) shall be deemed incompatible for + * the purposes of the Work and the provisions of the compatibility + * clause in Article 5 of the EUPL shall not apply. + * + * If using the Work as, or as part of, a network application, by + * including the attribution notice(s) required under Article 5 of the EUPL + * in the end user terms of the application under an appropriate heading, + * such notice(s) shall fulfill the requirements of that article. + * ********************************************************************* */ + +package fiftyone.pipeline.engines.services; + +import java.io.IOException; +import java.io.InterruptedIOException; +import java.net.InetAddress; +import java.net.Proxy; +import java.net.ProxySelector; +import java.net.SocketTimeoutException; +import java.net.URISyntaxException; +import java.net.URL; +import java.net.UnknownHostException; +import java.util.List; +import java.util.Locale; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executor; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +/** + * Looks up the host of a URL within a time limit. The connect timeout of an + * {@link java.net.HttpURLConnection} does not cover the name lookup, which + * waits as long as the system resolver does. Looking the host up here first + * leaves the address in the JVM's cache for the connection to use. Callers + * waiting on the same host share one lookup, so a resolver that does not + * answer holds one thread, not one for each request. + */ +final class HostLookup { + + /** + * Message of the exception thrown when a lookup takes longer than the + * timeout. The arguments are the host and the timeout in milliseconds. + */ + static final String MessageLookupTimedOut = + "Looking up the host '%s' took longer than the timeout of %d ms."; + + /** + * Finds the addresses of a host. + */ + interface Resolver { + InetAddress[] resolve(String host) throws UnknownHostException; + } + + /** + * The lookup {@link HttpClientDefault} uses, through the JVM's resolver. + */ + static final HostLookup DEFAULT = new HostLookup(InetAddress::getAllByName); + + private static final Executor LOOKUPS = Executors.newCachedThreadPool( + runnable -> { + Thread thread = new Thread(runnable, "51Degrees host lookup"); + thread.setDaemon(true); + return thread; + }); + + private final Resolver resolver; + + private final ConcurrentMap> + inProgress = new ConcurrentHashMap<>(); + + HostLookup(Resolver resolver) { + this.resolver = resolver; + } + + /** + * Look up the host of the URL, waiting at most the timeout. Nothing is + * looked up when the timeout is 0, which means no limit, or when a proxy + * makes the request, because the proxy looks up the host. + * @param url the URL to be requested + * @param timeoutMillis the longest wait in milliseconds + * @throws SocketTimeoutException if the lookup takes longer + * @throws UnknownHostException if the host has no address + * @throws IOException if the lookup fails in another way + */ + void lookUp(URL url, int timeoutMillis) throws IOException { + String host = url.getHost(); + if (timeoutMillis <= 0 || + host == null || + host.isEmpty() || + usesProxy(url)) { + return; + } + String key = host.toLowerCase(Locale.ROOT); + CompletableFuture started = new CompletableFuture<>(); + CompletableFuture lookup = + inProgress.putIfAbsent(key, started); + if (lookup == null) { + lookup = started; + start(key, host, started); + } + try { + lookup.get(timeoutMillis, TimeUnit.MILLISECONDS); + } catch (TimeoutException e) { + throw new SocketTimeoutException(String.format( + MessageLookupTimedOut, host, timeoutMillis)); + } catch (ExecutionException e) { + throw failure(host, e.getCause()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + InterruptedIOException interrupted = new InterruptedIOException( + "Interrupted while looking up the host '" + host + "'."); + interrupted.initCause(e); + throw interrupted; + } + } + + /** + * Start the lookup on its own thread. A finished lookup is removed from + * those in progress, so the next request looks up again, which the JVM's + * cache answers while the address is cached. + */ + private void start( + String key, + String host, + CompletableFuture lookup) { + try { + LOOKUPS.execute(() -> { + try { + lookup.complete(resolver.resolve(host)); + } catch (Throwable e) { + lookup.completeExceptionally(e); + } finally { + inProgress.remove(key, lookup); + } + }); + } catch (RuntimeException | Error e) { + inProgress.remove(key, lookup); + lookup.completeExceptionally(e); + throw e; + } + } + + /** + * The exception for a lookup that failed, thrown on the caller's thread + * with the lookup's own exception as the cause. + */ + private static IOException failure(String host, Throwable cause) { + if (cause instanceof UnknownHostException) { + UnknownHostException unknown = + new UnknownHostException(cause.getMessage()); + unknown.initCause(cause); + return unknown; + } + return new IOException( + "Looking up the host '" + host + "' failed.", cause); + } + + /** + * Whether the default proxy selector sends the request through a proxy. + * A URL it cannot read counts as one, so it is not looked up here. + */ + private static boolean usesProxy(URL url) { + ProxySelector selector = ProxySelector.getDefault(); + if (selector == null) { + return false; + } + List proxies; + try { + proxies = selector.select(url.toURI()); + } catch (URISyntaxException | IllegalArgumentException e) { + return true; + } + if (proxies == null) { + return false; + } + for (Proxy proxy : proxies) { + if (proxy.type() != Proxy.Type.DIRECT) { + return true; + } + } + return false; + } +} diff --git a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HttpClientDefault.java b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HttpClientDefault.java index 3dfaa5282..971718113 100644 --- a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HttpClientDefault.java +++ b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HttpClientDefault.java @@ -29,8 +29,22 @@ import java.net.URL; import java.util.Map; +/** + * Makes requests with {@link HttpURLConnection}. The connect timeout set on + * a connection also limits the lookup of its host, which the connection's + * own timeouts do not cover. + */ public class HttpClientDefault implements HttpClient { + private final HostLookup hostLookup; + + public HttpClientDefault() { + this(HostLookup.DEFAULT); + } + + HttpClientDefault(HostLookup hostLookup) { + this.hostLookup = hostLookup; + } @Override public HttpURLConnection connect(URL url) throws IOException { @@ -49,6 +63,7 @@ public String postData( connection.setRequestProperty(e.getKey(), e.getValue()); } } + lookUpHost(connection); if (data == null) { connection.setDoOutput(false); } else { @@ -86,6 +101,7 @@ public String getResponseString( connection.setRequestProperty(e.getKey(), e.getValue()); } } + lookUpHost(connection); BufferedReader in = new BufferedReader( new InputStreamReader(connection.getResponseCode() / 100 == 2 ? @@ -100,4 +116,12 @@ public String getResponseString( in.close(); return response.toString(); } + + /** + * Look up the connection's host within its connect timeout, before the + * connection makes its own lookup, which has no limit. + */ + private void lookUpHost(HttpURLConnection connection) throws IOException { + hostLookup.lookUp(connection.getURL(), connection.getConnectTimeout()); + } } diff --git a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/HostLookupTests.java b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/HostLookupTests.java new file mode 100644 index 000000000..155b632d8 --- /dev/null +++ b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/HostLookupTests.java @@ -0,0 +1,266 @@ +/* ********************************************************************* + * This Original Work is copyright of 51 Degrees Mobile Experts Limited. + * Copyright 2026 51 Degrees Mobile Experts Limited, Davidson House, + * Forbury Square, Reading, Berkshire, United Kingdom RG1 3EU. + * + * This Original Work is licensed under the European Union Public Licence + * (EUPL) v.1.2 and is subject to its terms as set out below. + * + * If a copy of the EUPL was not distributed with this file, You can obtain + * one at https://opensource.org/licenses/EUPL-1.2. + * + * The 'Compatible Licences' set out in the Appendix to the EUPL (as may be + * amended by the European Commission) shall be deemed incompatible for + * the purposes of the Work and the provisions of the compatibility + * clause in Article 5 of the EUPL shall not apply. + * + * If using the Work as, or as part of, a network application, by + * including the attribution notice(s) required under Article 5 of the EUPL + * in the end user terms of the application under an appropriate heading, + * such notice(s) shall fulfill the requirements of that article. + * ********************************************************************* */ + +package fiftyone.pipeline.engines.services; + +import org.junit.After; +import org.junit.Test; + +import java.io.IOException; +import java.net.HttpURLConnection; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.Proxy; +import java.net.ProxySelector; +import java.net.SocketAddress; +import java.net.SocketTimeoutException; +import java.net.URI; +import java.net.URL; +import java.net.UnknownHostException; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +/** + * Checks that a host lookup that does not answer fails within the connect + * timeout, as the connection's own timeouts do not cover the lookup. + */ +public class HostLookupTests { + + private static final String HOST = "unanswered.example"; + + /** The timeout given to each lookup. */ + private static final int TIMEOUT_MILLIS = 200; + + /** + * How long a resolver that does not answer holds a lookup unless the + * test releases it. It stands in for a system resolver that waits 30 + * seconds before giving up. + */ + private static final long UNANSWERED_MILLIS = 30000; + + /** + * The longest a timed out lookup may take. It is many times the timeout, + * so a slow runner passes, and far below the resolver's wait, so a + * lookup that waited for the resolver fails. + */ + private static final long LIMIT_MILLIS = 5000; + + /** The longest a test waits for another thread to reach a point. */ + private static final long WAIT_MILLIS = 10000; + + private final CountDownLatch release = new CountDownLatch(1); + private final CountDownLatch entered = new CountDownLatch(1); + private final AtomicInteger calls = new AtomicInteger(); + + @After + public void releaseResolver() { + release.countDown(); + } + + /** + * A resolver that holds each lookup until released, then answers with + * the loopback address, or fails if it is never released. + */ + private HostLookup.Resolver held() { + return host -> { + calls.incrementAndGet(); + entered.countDown(); + try { + if (release.await(UNANSWERED_MILLIS, TimeUnit.MILLISECONDS)) { + return new InetAddress[] { InetAddress.getLoopbackAddress() }; + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + throw new UnknownHostException(host); + }; + } + + private static URL url() throws IOException { + return new URL("https://" + HOST + "/api/v4/evidencekeys"); + } + + private static long millisSince(long start) { + return TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start); + } + + @Test + public void LookUp_ResolverDoesNotAnswer_TimesOutNamingTheHost() + throws Exception { + HostLookup lookup = new HostLookup(held()); + long start = System.nanoTime(); + try { + lookup.lookUp(url(), TIMEOUT_MILLIS); + fail("The lookup should have timed out."); + } catch (SocketTimeoutException e) { + long took = millisSince(start); + assertTrue("The lookup took " + took + " ms.", took < LIMIT_MILLIS); + assertEquals( + String.format(HostLookup.MessageLookupTimedOut, + HOST, TIMEOUT_MILLIS), + e.getMessage()); + } + } + + @Test + public void LookUp_UnknownHost_ThrowsUnknownHostWithTheCause() + throws Exception { + UnknownHostException unknown = new UnknownHostException(HOST); + HostLookup lookup = new HostLookup(host -> { + throw unknown; + }); + try { + lookup.lookUp(url(), TIMEOUT_MILLIS); + fail("The lookup should have failed."); + } catch (UnknownHostException e) { + assertEquals(HOST, e.getMessage()); + assertSame(unknown, e.getCause()); + } + } + + @Test + public void LookUp_NoTimeout_DoesNotLookUp() throws Exception { + new HostLookup(held()).lookUp(url(), 0); + assertEquals(0, calls.get()); + } + + @Test + public void LookUp_AfterAFailure_LooksUpAgain() throws Exception { + HostLookup lookup = new HostLookup(host -> { + calls.incrementAndGet(); + throw new UnknownHostException(host); + }); + for (int i = 0; i < 2; i++) { + try { + lookup.lookUp(url(), TIMEOUT_MILLIS); + fail("The lookup should have failed."); + } catch (UnknownHostException e) { + // Expected. + } + } + assertEquals(2, calls.get()); + } + + @Test + public void LookUp_SameHostAtOnce_SharesOneLookup() throws Exception { + HostLookup lookup = new HostLookup(held()); + AtomicReference firstFailure = new AtomicReference<>(); + AtomicReference secondFailure = new AtomicReference<>(); + Thread first = lookUpOnThread(lookup, firstFailure); + assertTrue(entered.await(WAIT_MILLIS, TimeUnit.MILLISECONDS)); + Thread second = lookUpOnThread(lookup, secondFailure); + // The second caller is waiting on the first one's lookup once it is + // in a timed wait. + long start = System.nanoTime(); + while (second.getState() != Thread.State.TIMED_WAITING && + millisSince(start) < WAIT_MILLIS) { + Thread.sleep(10); + } + release.countDown(); + first.join(WAIT_MILLIS); + second.join(WAIT_MILLIS); + assertNull(firstFailure.get()); + assertNull(secondFailure.get()); + assertEquals(1, calls.get()); + } + + private static Thread lookUpOnThread( + HostLookup lookup, + AtomicReference failure) { + Thread thread = new Thread(() -> { + try { + lookup.lookUp(url(), (int) WAIT_MILLIS); + } catch (Throwable e) { + failure.set(e); + } + }); + thread.start(); + return thread; + } + + @Test + public void LookUp_ThroughAProxy_DoesNotLookUp() throws Exception { + ProxySelector original = ProxySelector.getDefault(); + ProxySelector.setDefault(new ProxySelector() { + @Override + public List select(URI uri) { + return Collections.singletonList(new Proxy( + Proxy.Type.HTTP, + InetSocketAddress.createUnresolved("proxy.example", 8080))); + } + + @Override + public void connectFailed( + URI uri, SocketAddress address, IOException e) { + } + }); + try { + new HostLookup(held()).lookUp(url(), TIMEOUT_MILLIS); + } finally { + ProxySelector.setDefault(original); + } + assertEquals(0, calls.get()); + } + + @Test + public void GetResponseString_HostDoesNotAnswer_FailsWithinConnectTimeout() + throws Exception { + HttpClient client = new HttpClientDefault(new HostLookup(held())); + HttpURLConnection connection = client.connect(url()); + connection.setConnectTimeout(TIMEOUT_MILLIS); + long start = System.nanoTime(); + try { + client.getResponseString(connection); + fail("The request should have timed out."); + } catch (SocketTimeoutException e) { + long took = millisSince(start); + assertTrue("The request took " + took + " ms.", took < LIMIT_MILLIS); + } + } + + @Test + public void PostData_HostDoesNotAnswer_FailsWithinConnectTimeout() + throws Exception { + HttpClient client = new HttpClientDefault(new HostLookup(held())); + HttpURLConnection connection = client.connect(url()); + connection.setConnectTimeout(TIMEOUT_MILLIS); + long start = System.nanoTime(); + try { + client.postData( + connection, Collections.emptyMap(), new byte[] { 1 }); + fail("The request should have timed out."); + } catch (SocketTimeoutException e) { + long took = millisSince(start); + assertTrue("The request took " + took + " ms.", took < LIMIT_MILLIS); + } + } +} From 00171e5bc70587f185f828a48751e6fde6030f61 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Fri, 13 Nov 2026 14:04:09 +0000 Subject: [PATCH 2/5] Process nothing in the geo-location cloud engine when the cloud failed A failed cloud request leaves no response. GeoLocationCloudEngine read it anyway, so each failure added a NullPointerException from the JSON parser after the Cloud Request Engine's own error. It now logs a warning and processes nothing, as CloudAspectEngineBase does. --- .../flowelements/GeoLocationCloudEngine.java | 16 ++++++++++++++++ .../GeoLocationCloudEngineNullValueTests.java | 16 ++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java index 6c8eae436..60004c6df 100644 --- a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java +++ b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java @@ -48,6 +48,14 @@ public class GeoLocationCloudEngine extends CloudAspectEngineBase { + /** + * Warning logged when the Cloud Request Engine has no response to read. + * The argument is the name of this engine's class. + */ + static final String MessageNoCloudResponse = + "The %s did not process, as the Cloud Request Engine has no " + + "response. The Cloud Request Engine's error in the log says why."; + private CloudRequestEngine cloudRequestEngine = null; private volatile List properties; private final Object metadataLock = new Object(); @@ -137,6 +145,14 @@ protected void processEngine(FlowData data, CloudGeoData aspectData) { CloudRequestData requestData = data.getFromElement(cloudRequestEngine); String json = requestData.getJsonResponse(); + // A failed cloud request leaves no response. The Cloud Request + // Engine reports why, so this engine adds no error of its own. + if (json == null || json.isEmpty()) { + logger.warn(String.format( + MessageNoCloudResponse, getClass().getSimpleName())); + return; + } + JSONObject jsonObj = new JSONObject(json); JSONObject geoObj = getElementJson(jsonObj); diff --git a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java index 8bfe09c1b..3979c768a 100644 --- a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java +++ b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java @@ -142,6 +142,22 @@ public void SetNoValueReasons_ChangesNothing() throws Exception { } } + /** + * A failed cloud request leaves no response. The engine processes + * nothing and throws nothing, as the Cloud Request Engine's error says + * why. + */ + @Test + public void NoResponse_ProcessesNothing() throws Exception { + try (GeoLocationCloudEngine engine = buildEngine()) { + for (String response : new String[] { null, "" }) { + CloudGeoData data = process(engine, response); + + assertFalse(data.asKeyMap().containsKey("Town")); + } + } + } + private static void assertMissing(Runnable read) { try { read.run(); From cef731641fba33c4b2eb16f831a97cc894ea43ce Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Fri, 13 Nov 2026 14:07:53 +0000 Subject: [PATCH 3/5] Process nothing in every cloud engine when the cloud request failed GeoLocationCloudEngine, DeviceDetectionCloudEngine, IPIntelligenceCloudEngine and the developer example's SimpleCloudEngine override processEngine and read the Cloud Request Engine's response without the check that CloudAspectEngineBase makes. A failed cloud request leaves no response, so each failure added a NullPointerException from the JSON parser after the Cloud Request Engine's own error. Each now logs one warning and processes nothing. The message is shared as Constants.Messages.NoCloudResponse. --- .../DeviceDetectionCloudEngine.java | 10 ++++++++++ .../flowelements/ProductMissingTests.java | 20 +++++++++++++++++++ .../IPIntelligenceCloudEngine.java | 10 ++++++++++ .../flowelements/ProductMissingTests.java | 20 +++++++++++++++++++ .../flowelements/GeoLocationCloudEngine.java | 12 +++-------- .../cloudrequestengine/Constants.java | 10 ++++++++++ .../flowelements/SimpleCloudEngine.java | 10 ++++++++++ 7 files changed, 83 insertions(+), 9 deletions(-) diff --git a/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java b/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java index 400ca1dad..85ebcd913 100644 --- a/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java +++ b/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java @@ -41,6 +41,8 @@ import org.json.JSONObject; import org.slf4j.Logger; +import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.NoCloudResponse; + import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -138,6 +140,14 @@ protected void processEngine(FlowData data, DeviceDataCloud aspectData) { String json = ""; json = requestData.getJsonResponse(); + // A failed cloud request leaves no response. The Cloud Request + // Engine reports why, so this engine adds no error of its own. + if (json == null || json.isEmpty()) { + logger.warn(String.format( + NoCloudResponse, getClass().getSimpleName())); + return; + } + // Extract data from json to the aspectData instance. JSONObject jsonObj = new JSONObject(json); JSONObject deviceObj = getElementJson(jsonObj); diff --git a/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java b/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java index 2cd708eb6..00e1abf90 100644 --- a/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java +++ b/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java @@ -97,6 +97,26 @@ public void ProductMissing_ProcessLogsAndThrowsNothing() throws Exception { } } + /** + * A failed cloud request leaves no response. The engine processes + * nothing, throws nothing and logs one warning, as the Cloud Request + * Engine's error says why. + */ + @Test + public void NoResponse_ProcessesNothing() throws Exception { + try (DeviceDetectionCloudEngine engine = buildEngine("device", "hardwarevendor")) { + for (String response : new String[] { null, "" }) { + DeviceDataCloud data = process(engine, response); + assertFalse(data.asKeyMap().containsKey("hardwarevendor")); + } + List logged = warningsAndErrors(); + assertEquals(logged.toString(), 2, logged.size()); + for (String line : logged) { + assertTrue(line, line.startsWith("warn ")); + } + } + } + /** * With the Device Detection product included, a response that holds no * part for it leaves the element data without values and throws diff --git a/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java b/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java index 99b84cbb3..8ca08e569 100644 --- a/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java +++ b/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java @@ -41,6 +41,8 @@ import org.json.JSONObject; import org.slf4j.Logger; +import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.NoCloudResponse; + import fiftyone.pipeline.core.data.IWeightedValue; import fiftyone.pipeline.core.data.WeightedValue; import fiftyone.pipeline.core.data.WktString; @@ -150,6 +152,14 @@ protected void processEngine(FlowData data, IPIntelligenceDataCloud aspectData) String json = ""; json = requestData.getJsonResponse(); + // A failed cloud request leaves no response. The Cloud Request + // Engine reports why, so this engine adds no error of its own. + if (json == null || json.isEmpty()) { + logger.warn(String.format( + NoCloudResponse, getClass().getSimpleName())); + return; + } + // Extract data from json to the aspectData instance. JSONObject jsonObj = new JSONObject(json); JSONObject ipiObj = getElementJson(jsonObj); diff --git a/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java b/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java index a597f4662..924a476bb 100644 --- a/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java +++ b/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java @@ -97,6 +97,26 @@ public void ProductMissing_ProcessLogsAndThrowsNothing() throws Exception { } } + /** + * A failed cloud request leaves no response. The engine processes + * nothing, throws nothing and logs one warning, as the Cloud Request + * Engine's error says why. + */ + @Test + public void NoResponse_ProcessesNothing() throws Exception { + try (IPIntelligenceCloudEngine engine = buildEngine("ip", "registeredname")) { + for (String response : new String[] { null, "" }) { + IPIntelligenceDataCloud data = process(engine, response); + assertFalse(data.asKeyMap().containsKey("registeredname")); + } + List logged = warningsAndErrors(); + assertEquals(logged.toString(), 2, logged.size()); + for (String line : logged) { + assertTrue(line, line.startsWith("warn ")); + } + } + } + /** * With the IP Intelligence product included, a response that holds no * part for it leaves the element data without values and throws diff --git a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java index 60004c6df..10da0267b 100644 --- a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java +++ b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java @@ -43,19 +43,13 @@ import org.json.JSONObject; import org.slf4j.Logger; +import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.NoCloudResponse; + import java.util.*; import org.json.JSONArray; public class GeoLocationCloudEngine extends CloudAspectEngineBase { - /** - * Warning logged when the Cloud Request Engine has no response to read. - * The argument is the name of this engine's class. - */ - static final String MessageNoCloudResponse = - "The %s did not process, as the Cloud Request Engine has no " + - "response. The Cloud Request Engine's error in the log says why."; - private CloudRequestEngine cloudRequestEngine = null; private volatile List properties; private final Object metadataLock = new Object(); @@ -149,7 +143,7 @@ protected void processEngine(FlowData data, CloudGeoData aspectData) { // Engine reports why, so this engine adds no error of its own. if (json == null || json.isEmpty()) { logger.warn(String.format( - MessageNoCloudResponse, getClass().getSimpleName())); + NoCloudResponse, getClass().getSimpleName())); return; } diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java index eebb82eb4..73b0756a9 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java @@ -58,6 +58,16 @@ public class Messages { public static final String EvidenceConflict = "'%s:%s' evidence conflicts with %s"; + + /** + * Warning a cloud engine logs when the Cloud Request Engine has no + * response for it to read. The argument is the engine's class name. + */ + public static final String NoCloudResponse = + "The %s did not process, as the Cloud Request Engine has no " + + "response. The Cloud Request Engine's error in the log says " + + "why."; + public static final String MessageNoDataInResponse = "No data in response from cloud service at '%s'"; public static final String MessageErrorCodeReturned = diff --git a/pipeline/pipeline.developer-examples/pipeline.developer-examples.cloud-engine/src/main/java/pipeline/developerexamples/cloudengine/flowelements/SimpleCloudEngine.java b/pipeline/pipeline.developer-examples/pipeline.developer-examples.cloud-engine/src/main/java/pipeline/developerexamples/cloudengine/flowelements/SimpleCloudEngine.java index 89cd49481..cc6d14970 100644 --- a/pipeline/pipeline.developer-examples/pipeline.developer-examples.cloud-engine/src/main/java/pipeline/developerexamples/cloudengine/flowelements/SimpleCloudEngine.java +++ b/pipeline/pipeline.developer-examples/pipeline.developer-examples.cloud-engine/src/main/java/pipeline/developerexamples/cloudengine/flowelements/SimpleCloudEngine.java @@ -34,6 +34,8 @@ import org.json.JSONObject; import org.slf4j.Logger; +import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.NoCloudResponse; + import java.util.ArrayList; import java.util.List; @@ -90,6 +92,14 @@ protected void processEngine(FlowData data, StarSignData aspectData) { CloudRequestData requestData = data.getFromElement(cloudRequestEngine); String json = requestData.getJsonResponse(); + // A failed cloud request leaves no response. The Cloud Request + // Engine reports why, so this engine adds no error of its own. + if (json == null || json.isEmpty()) { + logger.warn(String.format( + NoCloudResponse, getClass().getSimpleName())); + return; + } + // Extract data from json to the aspectData instance. A cloud // service answers only for the products the resource key covers, // so a response without the star sign product leaves the value From dd8d73562fcd91539848e712e0608206bfab67c1 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Fri, 13 Nov 2026 14:18:29 +0000 Subject: [PATCH 4/5] Build the test URL without the constructor Java 20 deprecates --- .../fiftyone/pipeline/engines/services/HostLookupTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/HostLookupTests.java b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/HostLookupTests.java index 155b632d8..06bddbe6b 100644 --- a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/HostLookupTests.java +++ b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/HostLookupTests.java @@ -106,7 +106,7 @@ private HostLookup.Resolver held() { } private static URL url() throws IOException { - return new URL("https://" + HOST + "/api/v4/evidencekeys"); + return URI.create("https://" + HOST + "/api/v4/evidencekeys").toURL(); } private static long millisSince(long start) { From 028c23f79ee5cb371918719e30d9a75fcbd2921b Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Fri, 13 Nov 2026 20:28:22 +0000 Subject: [PATCH 5/5] Say what the lookup cache costs and share the no response warning HostLookup relies on the JVM's address cache, so its comment now says the two limits that follow. With networkaddress.cache.ttl at 0 the connection looks the host up again with no limit, and a request the connection would send on a kept-alive socket is looked up too, so it fails once the cached address expires and the resolver does not answer. HttpClientDefault says that a subclass connecting through a proxy of its own is still looked up. CloudAspectEngineBase logs Constants.Messages.NoCloudResponse, as the four engines that override processEngine do, and the geo-location test checks the warning as the Device Detection and IP Intelligence tests do. --- .../GeoLocationCloudEngineNullValueTests.java | 26 ++++++++++++++++--- .../flowelements/CloudAspectEngineBase.java | 8 +++--- .../pipeline/engines/services/HostLookup.java | 9 +++++++ .../engines/services/HttpClientDefault.java | 6 ++++- 4 files changed, 39 insertions(+), 10 deletions(-) diff --git a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java index 3979c768a..74391dba6 100644 --- a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java +++ b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java @@ -36,6 +36,7 @@ import fiftyone.pipeline.engines.services.MissingPropertyServiceDefault; import org.junit.Test; import org.slf4j.ILoggerFactory; +import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.util.Arrays; @@ -46,8 +47,12 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.NoCloudResponse; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; /** @@ -144,18 +149,23 @@ public void SetNoValueReasons_ChangesNothing() throws Exception { /** * A failed cloud request leaves no response. The engine processes - * nothing and throws nothing, as the Cloud Request Engine's error says - * why. + * nothing, throws nothing and logs one warning each time, as the Cloud + * Request Engine's error says why. */ @Test public void NoResponse_ProcessesNothing() throws Exception { - try (GeoLocationCloudEngine engine = buildEngine()) { + Logger logger = mock(Logger.class); + ILoggerFactory warnings = mock(ILoggerFactory.class); + when(warnings.getLogger(anyString())).thenReturn(logger); + try (GeoLocationCloudEngine engine = buildEngine(warnings)) { for (String response : new String[] { null, "" }) { CloudGeoData data = process(engine, response); assertFalse(data.asKeyMap().containsKey("Town")); } } + verify(logger, times(2)).warn(String.format( + NoCloudResponse, GeoLocationCloudEngine.class.getSimpleName())); } private static void assertMissing(Runnable read) { @@ -199,8 +209,16 @@ private static void assertValue(Object expected, Object value) * the Suburb and Town properties. */ private GeoLocationCloudEngine buildEngine() throws Exception { + return buildEngine(loggerFactory); + } + + /** + * The same engine, logging through the given factory. + */ + private GeoLocationCloudEngine buildEngine(ILoggerFactory engineLoggers) + throws Exception { GeoLocationCloudEngine engine = - new GeoLocationCloudEngineBuilder(loggerFactory) + new GeoLocationCloudEngineBuilder(engineLoggers) .build(Enums.GeoLocationProvider.FiftyOneDegrees); ProductMetaData product = new ProductMetaData(); diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java index 80b39e9a6..fd037341a 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java @@ -23,6 +23,7 @@ package fiftyone.pipeline.cloudrequestengine.flowelements; import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.MessageProductNotInResourceKey; +import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.NoCloudResponse; import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.ProcessCloudEngineNotImplemented; import fiftyone.pipeline.cloudrequestengine.NotImplementedException; @@ -432,11 +433,8 @@ protected void processEngine(FlowData data, TData aspectData) { if (json != null && !json.isEmpty()) { processCloudEngine(data, aspectData, json); } else { - logger.warn("The " + this.getClass().getSimpleName() + " did not process " + - "as the JSON response from the CloudRequestEngine was null " + - "or empty. Please refer to errors generated by the " + - "CloudRequestEngine in the logs as this indicates an error " + - "occurred there."); + logger.warn(String.format( + NoCloudResponse, getClass().getSimpleName())); } } diff --git a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HostLookup.java b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HostLookup.java index 0fae02581..fc4939cad 100644 --- a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HostLookup.java +++ b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HostLookup.java @@ -49,6 +49,15 @@ * leaves the address in the JVM's cache for the connection to use. Callers * waiting on the same host share one lookup, so a resolver that does not * answer holds one thread, not one for each request. + *

+ * Two limits follow from relying on that cache. With the security property + * {@code networkaddress.cache.ttl} set to 0 nothing is cached, so the + * connection looks the host up again itself, with no limit. And every + * request is looked up here, including one the connection would send on a + * kept-alive socket with no lookup at all, so once the cached address + * expires, 30 seconds after it was found by default, a resolver that does + * not answer fails that request although the open socket could have served + * it. */ final class HostLookup { diff --git a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HttpClientDefault.java b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HttpClientDefault.java index 971718113..4759da850 100644 --- a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HttpClientDefault.java +++ b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/HttpClientDefault.java @@ -32,7 +32,11 @@ /** * Makes requests with {@link HttpURLConnection}. The connect timeout set on * a connection also limits the lookup of its host, which the connection's - * own timeouts do not cover. + * own timeouts do not cover. A host is not looked up when the default + * {@link java.net.ProxySelector} sends its request through a proxy. A + * subclass whose {@link #connect(URL)} opens the connection through a proxy + * of its own still has the host looked up here, so its requests fail when + * the host cannot be found locally although the proxy could reach it. */ public class HttpClientDefault implements HttpClient {