[jwrosewell]

This draft closes two gaps in the cloud packages. Requests to the cloud service get the timeout the specification gives, 2 seconds unless the new timeoutSeconds setting sets another, where today they have none. And a cloud engine takes its Properties from the Cloud Request Engine when it joins the Pipeline, where today it takes them only when it first processes.

The problem

File and line at rc 36b71fde.

1. No timeout on cloud requests. HttpClient::makeCloudRequest sets no timeout. With curl (pipeline/cloudrequestengine/src/HttpClient.php lines 85 to 100) a request waits as long as the cloud service takes, and without curl (lines 50 to 59) it waits for PHP's default_socket_timeout. The Cloud Request Engine has no setting for it (CloudRequestEngine.php lines 81 to 136). The red run below shows a request to a server that answers after 8 seconds waiting the full 8 seconds.
2. Cloud engines take their Properties on first use. CloudEngine::onRegistration (CloudEngine.php lines 62 to 69) only checks that the Pipeline has a Cloud Request Engine, and processInternal takes the Properties (lines 82 to 85). So until the first request is processed, a cloud engine reports no Properties, even when the Cloud Request Engine already has them.

What the specification says

All at specifications 706de1f0.

The timeout. The Configuration options table of pipeline-elements/cloud-request-engine.md, line 506, which lines 492 and 493 introduce as "the configuration options that are unique to this Engine", has this row.

| TimeoutSeconds | integer | 2 | The timeout to use when making requests to the cloud service |

The setting here is named timeoutSeconds, in the camelCase of the engine's other settings (resourceKey, licenseKey, cloudEndPoint, cloudRequestOrigin). A timeout of 0 or less is refused, under James Rosewell's rule of 12 November, "Let's make it really hard for people to configure us badly", because curl reads 0 as no timeout at all.

The Properties. pipeline-elements/cloud-aspect-engine.md lines 40 to 44, under Start-up activity.

The Cloud Request Engine normally resolves this information when it is built, so a Cloud Aspect Engine built after it can read the accessible Properties and populate its Property metadata at build time.

Lines 46 to 51 cover the case where it has not.

A Cloud Aspect Engine therefore MUST treat the Property metadata as failable, lazily-completed data: it MUST tolerate the metadata being temporarily absent and MUST NOT permanently cache an empty result obtained before the Cloud Request Engine's discovery has completed.

What changed

1. The timeout. HttpClient has setTimeoutSeconds(float) and getTimeoutSeconds(), 2 seconds (Constants::TIMEOUT_SECONDS_DEFAULT) unless set. With curl it sets CURLOPT_TIMEOUT_MS, which covers the whole request, connecting included, and CURLOPT_NOSIGNAL, so that a timeout below one second works too. Without curl it sets the stream context's timeout, which PHP applies to reading the answer. The Cloud Request Engine reads a new timeoutSeconds setting and sets it on the client it uses. A value that is not a number greater than 0 is refused when the engine is built, and setTimeoutSeconds refuses one of 0 or less, both with Constants::MESSAGE_TIMEOUT_NOT_POSITIVE. A client passed as httpClient keeps its own timeout unless the setting is given. A request that times out fails as one that could not be made, with the CloudRequestException that HttpClient already raises for a failed connection.
2. The Properties. CloudEngine::onRegistration takes the engine's Properties from the Cloud Request Engine when it has a product for the engine's Data Key, and leaves them as they are when it has none. processInternal still takes them on first use, as before, so an engine that joined before discovery completed gets them then, and an empty result is never kept. Every cloud engine here, Device Detection, hardware profile, location and IP Intelligence, inherits it.

On rc the Cloud Request Engine asks the cloud service for its Properties only when it first processes, so the second change has an effect when the engine already has them, for example after getEngineProperties(). #128 makes the engine ask when it is built, and with it this becomes the normal case. The two merge cleanly, and the discovery requests #128 makes at build time go through the same client, so they take the timeout too.

Before you upgrade

1. Cloud requests stop after 2 seconds unless set otherwise. Before, with curl a request waited as long as the cloud service took, and without curl it waited for PHP's default_socket_timeout, 60 seconds unless changed. A request that takes longer than the timeout now fails with CloudRequestException, "The request to the cloud service at '...' could not be made", followed by the reason curl gives, and the error is recorded against the Cloud Request Engine as any other failed request is. Set timeoutSeconds in the Cloud Request Engine's settings to wait longer.
2. Without curl, connecting is not covered. PHP applies the stream context's timeout to reading the answer, so the time to connect still follows default_socket_timeout.
3. A timeoutSeconds of 0 or less, or one that is not a number, is refused with InvalidArgumentException when the engine is built, and HttpClient::setTimeoutSeconds refuses 0 or less in the same way. An empty value of the setting counts as absent.
4. A cloud engine can now have its Properties before the first request, where the Cloud Request Engine has them when the Pipeline is built. Code that read an engine's getProperties() before processing and expected nothing will now find them.
5. New public names, the timeoutSeconds setting, HttpClient::setTimeoutSeconds(), HttpClient::getTimeoutSeconds(), Constants::TIMEOUT_SECONDS_DEFAULT and Constants::MESSAGE_TIMEOUT_NOT_POSITIVE. Nothing is removed or renamed.

Tests

Two new test files in the cloud request engine's Unit suite, both written from the sentences above. pipeline/cloudrequestengine/tests/TimeoutTests.php (6 tests) starts a local PHP server that answers each request only after 8 seconds and checks that a request fails with "could not be made" no earlier than 0.25 seconds before its timeout and less than 4 seconds after it, with the default, with setTimeoutSeconds, and with the engine's timeoutSeconds setting. It also checks that a given client takes the setting, that the engine refuses 0, -1 and 'two', and that setTimeoutSeconds refuses 0 and -1. pipeline/cloudrequestengine/tests/AspectEnginePropertiesTests.php (3 tests) checks that a cloud engine has its Properties once the Pipeline is built, that it builds without them and takes them on first use when the Cloud Request Engine has none yet, and that a missing product does not fail the build. The last two pass on rc too, and check that those cases keep working.

The red and green runs use a throwaway workflow that installs the packages from the checkout as CI does (Install-RootComposer and Set-RootAutoload from ci/packages.ps1) and runs PHPUnit 9.6.34 with --fail-on-warning, as CI does.

• Red, run 43632393515 on tmp/fixer-dx-red 4cb3e2e, which is rc with the test commit and the workflow, Ubuntu with PHP 8.5. It failed, as it should, with "Tests: 8, Assertions: 7, Errors: 2, Failures: 4".
  • The default and the setting. "The request waited 8.0 seconds for the answer, where it should have stopped after 2.0 seconds." and the same after 1.0 seconds for timeoutSeconds.
  • "Call to undefined method fiftyone\pipeline\cloudrequestengine\HttpClient::setTimeoutSeconds()" and the same for getTimeoutSeconds().
  • "A timeoutSeconds of 0 was not refused."
  • testACloudEngineHasItsPropertiesWhenItJoinsThePipeline with "Failed asserting that an array has the key 'value'."
  • testSetTimeoutSecondsRefusesZeroOrLess came after the red run, with the refusal in setTimeoutSeconds it tests. On rc the method does not exist, as the red run shows for the other tests that call it.
• Green, run 43634642496 on tmp/fixer-dx-green 70def13, which is this head with the workflow. It ran on Ubuntu with PHP 7.4, the lowest the package supports, and 8.5, the highest, and on Windows with PHP 8.3, because the timeout tests start a local PHP server as a process of its own. All three jobs passed.
  • Cloud request engine Unit suite, which holds both new files, "OK (99 tests, 435 assertions)" on each.
  • Device Detection cloud Unit suite, whose engines inherit the change to onRegistration, "OK (29 tests, 203 assertions)" on each.
  • IP Intelligence tests/IpIntelligenceCloudTests.php, "Tests: 8, Assertions: 25, Skipped: 1" on PHP 8.5. IP Intelligence needs PHP 8.0 and builds an extension, so it is not installed on PHP 7.4 or on Windows, as in CI.
• An earlier green run, run 43633530480, ran the head before setTimeoutSeconds refused 0 or less. The cloud request engine suite gave "OK (98 tests, 432 assertions)" on all three jobs, and the Windows job failed only because the workflow ran the IP Intelligence tests there, where the package is not installed.

The timing limits and what each runner measured

TimeoutTests fails a request that stops more than EARLY_SECONDS, 0.25 seconds, before its timeout, or LATE_SECONDS, 4 seconds, after it, and gives the local server that stands in for a slow cloud service SERVER_START_SECONDS, 10 seconds, to start (TimeoutTests.php lines 41 to 53). Commit 8a37ddfb makes each test write to standard error the time each timed-out request and the server took, beside those limits, so every run records them. Each figure below is from the job's own log. Each job has three timed requests, of a 2 second and two 1 second timeouts.

Runner	Run	Requests stopped after (timeout 2 s, 1 s, 1 s)	Server started after
Ubuntu, PHP 7.4 to 8.4	full build 43688679819, six jobs	2.002 s, 1.001 s, 1.001 s in every job	0.100 s in every job
Ubuntu, PHP 8.5	full build 43688679819	2.003 s, 1.001 s, 1.001 s	0.100 s
Windows, PHP 8.3	full build 43688679819	2.001 s, and 1.001 s and 1.007 s	0.314 s
Ubuntu 24.04, PHP 8.5	throwaway 43688673516	2.003 s, 1.002 s, 1.002 s	0.100 s
Windows, PHP 8.3	throwaway 43688673516	2.004 s, 1.007 s, 1.001 s	0.302 s
macOS, PHP 8.5	throwaway 43688673516	2.014 s, 1.002 s, 1.001 s	0.135 s

The latest any request stopped was 0.014 seconds after its timeout, on macOS, against a limit of 4 seconds, no request stopped early, and the slowest server start was 0.314 seconds, on Windows, against 10 seconds. The macOS figures come from the throwaway run because a draft's full build leaves macOS out, and the rc head's release build, which has macOS, prints them too.

The tests that passed on rc

The last two AspectEnginePropertiesTests, testACloudEngineTakesItsPropertiesLaterWhenThereAreNoneYet and testAMissingProductDoesNotFailTheBuild, pass on rc too. Run 43688673516, job 132734460487, at 76701449, which is this draft's head 8a37ddf with a temporary commit holding a workflow and two patch files, ran the file unchanged, where all three tests passed, then under each change.

Throwaway change	Test that failed
processInternal no longer takes the Properties	testACloudEngineTakesItsPropertiesLaterWhenThereAreNoneYet only
onRegistration throws when the Cloud Request Engine has Properties but none for this engine	testAMissingProductDoesNotFailTheBuild only

Other drafts that change the same files

git merge-tree of this head with each open draft finds one conflict.

• #134 also changes CloudEngine::onRegistration, where it finds the Cloud Request Engine by its new Data Key, cloud-response, or by cloud. Whichever merges second keeps that lookup and takes the Properties from the engine it finds.
• #128 (HttpClient.php, CloudRequestEngine.php, Constants.php, phpunit.xml), #127, #121 (phpunit.xml) and #113 merge with it cleanly.
• #128 makes the Cloud Request Engine ask the cloud service for discovery when it is built. testAGivenHttpClientTakesTheSetting built its engines with the test's made-up resource key against the real cloud service, so in a build of rc with every open PHP draft merged, run 43683733486, job 132719933541 (Ubuntu, PHP 7.4), the build of the engine failed with "'resource_key' could not be read as a valid resource key". Commit bd68e437 points those engines at http://127.0.0.1:1/, where nothing listens, so a request made while an engine is built fails at once, which Ask the cloud service for discovery when the Cloud Request Engine is built, build it from saved results, and log the cloud service's warnings #128 does not let fail the build, and the test checks the client's timeout with or without Ask the cloud service for discovery when the Cloud Request Engine is built, build it from saved results, and log the cloud service's warnings #128. Run 43687155017, on a composite of rc 44dbd4e with every open PHP draft merged, these changes included (85871de5 on a throwaway branch), passed the Unit suites of pipeline/core, pipeline/engines and pipeline/cloudrequestengine on Ubuntu 24.04 with PHP 8.1 and 8.5, this test among them.

Release

This draft changes package code in fiftyone.pipeline.cloudrequestengine only, so that package takes a new version. Under the release plan of #37, the packages that depend on it are released with it.

Build

Full build run 43688679819 passed on this draft's head 8a37ddfb. The build ran from rc at bee9ddd8. The run printed 9 of 9 Setting 'PullRequestSha' lines on 8a37ddf, and Configure printed "Left out 5 macOS entries". All eight Build and Test jobs passed, Ubuntu with PHP 7.4 and 8.0 to 8.5 and Windows with PHP 8.3. Attempt 1's Ubuntu PHP 8.5 job failed in Fetch Assets, before any test ran, when the download of the free ASN file stalled (the data repository's script reported its stall as a formatting error, data#13), and that job was run again as attempt 2.

macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.

What remains

1. Recovery after trouble with the cloud service. The timeout is the first part of the Recovery Mode that the specification describes, which counts a request that did not complete, such as a timeout, towards a recovery period. PHP has no recovery period yet, and this draft does not add one. Raised as #157, which waits on specifications#176, as #128 does.