[#163] Parse CREST query results on the calling thread, reuse connections and stop the per-request 401

[maximthomas]

Fixes #163.

ScriptedCRESTConnector no longer runs query result handlers on the HTTP client's I/O thread. It now stops a query when the handler returns false, takes a 401 only on the first BASIC request to a host, and reuses pooled connections instead of sending Connection: close.

• Query handlers run on the caller's thread. The synchronous AbstractRemoteConnection.query() no longer delegates to queryAsync(). The I/O thread only buffers the response (BufferedResponseConsumer). The caller then parses the body and calls the handler. Previously, the script's handleResource and, with producerBufferSize 0 or a full buffer, the ICF ResultsHandler ran on an I/O dispatcher thread. A synchronous CREST call made from such a handler hung as soon as its connection landed on that dispatcher. The exception mapping of query() is unchanged:

  • a ResourceException reaches the caller as is;
  • any other Throwable raised before the response arrives becomes an InternalServerErrorException;
  • an exception from parsing or from the handler goes through adapt;
  • an interrupted wait becomes a ServiceUnavailableException.

  queryAsync and the public QueryResultResponseHandler are unchanged. A queryAsync handler still runs on the I/O thread. Its Javadoc now says so, and says that query() does not call it, so a subclass that overrides queryAsync to change the request must override query as well. The body is still buffered in full.

• A false from the handler stops the query. StreamingJsonSlurper stops parsing at once and skips its callback. parseQueryResponse then returns a QueryResponse without a paged-results cookie and leaves the rest of the response unread. This also applies to queryAsync, which uses the same parseQueryResponse.

• Only the first BASIC request takes a 401. After the customizer's beforeRequest, ScriptedCRESTConfiguration.execute installs a shared SharedBasicAuthCache into a request context that has no AuthCache. The first successful challenge fills it, and later requests send their credentials up front.

  • The cache keeps only BasicSchemes. A Digest scheme carries a nonce count that concurrent requests must not share.
  • Its methods are synchronized, because BasicAuthCache is a plain HashMap before HttpClient 4.4.
  • get returns a new BasicScheme for every request, with the cached scheme's credentials charset and proxy/target state. On httpclient 4.3.x, which the bundle imports, BasicScheme encodes through a shared commons-codec Base64 instance and is not thread-safe. One instance shared by concurrent requests produced wrong Authorization headers. The copy does not carry the realm: the Basic header does not use it, and preemptive authentication looks up the credentials for any realm.
  • A context that already has an AuthCache is left alone. BASIC_PREEMPTIVE puts one there: the default customizer now fills a SharedBasicAuthCache with a BasicScheme instead of a plain BasicAuthCache, so these requests get their own copy too. Before, all of them shared one BasicScheme, and authFailed and authSucceeded changed the cache's HashMap on the I/O threads.
  • execute now builds the client, which runs the customizer's init, before it runs beforeRequest. init fills the propertyBag that beforeRequest reads, so the first request of a client went out without the BASIC_PREEMPTIVE AuthCache and without the customizer's cookie store, and with BASIC_PREEMPTIVE it took a 401.

• Connections are reused. The Connection: close, TE header is gone.

  • It came with OPENIDM-2604 (connections to OpenDJ occasionally timed out).
  • getHttpAsyncClient() now sets IdleKeepAliveStrategy on the builder before the customizer's init closure runs, so a custom customizer can still replace it.
  • A pooled connection is reused for as long as the server's Keep-Alive: timeout allows, and for at most 10 s. That is below the idle timeouts of common servers (Jetty 30 s, Tomcat 20 s). A timeout=0 expires it at once (1 ms), because the pool keeps a duration of 0 forever. The pool closes an expired connection the next time it would lease it. The I/O reactor closes a connection as soon as the server closes it.

• The default customizer limits connect and pool waits. A connect attempt fails after 10 s. A request that is still waiting for a pooled connection after 60 s fails. The pool checks that deadline only when it releases a connection or a connect attempt ends, so a wait can run over. The socket timeout stays unlimited, because a long query may send nothing until its result is ready. The proxy branch now copies these settings and adds the proxy. Before, it replaced the whole RequestConfig.

Behaviour changes:

• query handlers run on the caller's thread, and an Error thrown by a handler now reaches the caller (it used to stop the dispatcher and hang the caller);
• a handler is no longer called after the caller is interrupted;
• a false from a handler ends the results, without a cookie;
• one TCP connection is reused across requests, and only the first BASIC request to a host takes a 401; with BASIC_PREEMPTIVE, the first request of a client no longer takes one either;
• with BASIC, the requests after the first look up their credentials for any realm of the host and port, as BASIC_PREEMPTIVE already did. A customizer that registers credentials per realm on one host and port may then send the wrong ones, and that request fails on the 401 without a retry. builder.disableAuthCaching() in init, or an own AuthCache set in beforeRequest, brings back a challenge on every request;
• connect attempts time out after 10 s, and waits for a pooled connection after about 60 s.

Remaining risk: a server can close an idle connection just as the client reuses it. That request then fails, because httpasyncclient 4.0.2 does not retry. The 10 s cap keeps reuse below the idle timeouts of common servers.

Not changed:

• the REST customizer's BASIC setup;
• streaming of response bodies;
• the I/O thread on which read, create, update, patch, delete and action parse their responses (none of them calls user code).

Test strength

arm	case	evidence
query() parses on the caller	ScriptedCRESTConnectorTest#testSearchHandlerRunsOnTheCallingThread	red at 78c1e16 (handler on I/O dispatcher 4)
nested sync call from a handler	#testNestedGetObjectFromTheSearchHandlerCompletes	red at 78c1e16 (60 s time-out); at the head it passes against the query() revert, because keep-alive keeps the nested calls on one pooled connection off the handler's dispatcher — #testSearchHandlerRunsOnTheCallingThread is the pin
handler exception → InternalServerErrorException	#testHandlerExceptionReachesTheCallerAsInternalServerError	kept behaviour, green at 78c1e16
HTTP 404 → NotFoundException	#testQueryOfAMissingResourceFailsWithNotFound	kept behaviour, green at 78c1e16
Error from execute → InternalServerErrorException	AbstractRemoteConnectionTest#testQueryWrapsAnErrorFromExecuteInInternalServerError	kept behaviour, green at 78c1e16
interrupted wait → ServiceUnavailableException	#testInterruptedQueryFailsWithServiceUnavailable	kept behaviour, green at 78c1e16; with the interrupt() removed it fails on its 10 s timeOut instead of hanging
transport ResourceException passes through	#testQueryPassesATransportResourceExceptionThrough	kept behaviour; mutant throw adapt(t) → throw new InternalServerErrorException(t) in the wait phase killed
false stops the query	ScriptedCRESTConnectorTest#testSearchStopsWhenTheHandlerReturnsFalse	red at 8ae543a (expected [1] but found [2])
slurper stop, nested arrays, reset	StreamingJsonSlurperTest (5)	compile-red at 8ae543a; deleting the stop check in parseObject fails 4 of them
no 401 after the first request	ScriptedCRESTConnectorTest#testOperationsAfterTheFirstDoNotTakeA401	red at 53925d1 (expected [0] but found [20])
Basic-only cache	SharedBasicAuthCacheTest keep/drop/remove	compile-red at 53925d1
per-request scheme copy	SharedBasicAuthCacheTest#testGivesEachRequestItsOwnScheme	red at 509912a
copy keeps proxy state and charset	#testCopyKeepsTheProxyState, #testCopyKeepsTheCredentialsCharset	mutant get → new BasicScheme() killed
BASIC_PREEMPTIVE scheme copy per request	ScriptedCRESTConfigurationTest#testPreemptiveBasicGivesEachRequestItsOwnScheme	red at c58eb0d (expected not same [BASIC] but found [BASIC]); dropping the pre-filled scheme fails it too
BASIC_PREEMPTIVE first request, init before beforeRequest, AuthCache guard	ScriptedCRESTConnectorTest#testPreemptiveBasicNeverTakesA401	red at c58eb0d and 4d8eb37 (expected [0] but found [1]); mutants if (true) for the guard and dropping the pre-filled scheme killed
default and proxy timeouts	ScriptedCRESTConfigurationTest#testDefaultCustomizerBoundsConnectAndPoolWaits, #testDefaultCustomizerKeepsTheTimeoutsWithAProxy	red at bd58232 (expected [10000] but found [-1])
keep-alive cap, timeout=0	IdleKeepAliveStrategyTest (4)	compile-red at facd1f2; #testExpiresAZeroAnnouncedTimeoutAtOnce red at ff4bc0f (expected [1] but found [10000])
strategy wired into the client	ScriptedCRESTConfigurationTest#testClientDropsConnectionsAfterTenIdleSeconds	red at facd1f2 (DefaultConnectionKeepAliveStrategy)
one connection reused	ScriptedCRESTConnectorTest#testOperationsReuseOneConnection	red at facd1f2 (expected [1] but found [20])

Not pinned by a test:

• the synchronized on SharedBasicAuthCache;
• setting the keep-alive strategy before the customizer rather than after it.

mvn -pl OpenICF-groovy-connector test: Tests run: 175, Failures: 0, Errors: 0, Skipped: 42. The 42 skipped tests are the five *SampleTest classes, which need a private configuration.

[vharseko]

BASIC_PREEMPTIVE keeps the defect this PR fixes for BASIC (pre-existing, not a regression)

CustomizerScript.groovy:138 puts one new BasicScheme() into a plain BasicAuthCache in the propertyBag, and execute leaves that context alone. RequestAuthCache (4.3.5) hands that one instance to every request, so concurrent authenticate() calls share its Base64 codec. The commons-codec 1.6 embedded in httpclient-osgi 4.3.5 keeps buffer, pos and bitWorkArea as instance fields, which is the cause the description gives for the per-request copy. After a 401, processChallenge() runs on the shared instance and authFailed removes it from the HashMap. The next successful challenge puts a new scheme into the same unsynchronised map from an I/O thread. CREST_SAMPLE uses BASIC_PREEMPTIVE, and it is one of the two options #163 proposes. Filling a SharedBasicAuthCache instead of BasicAuthCache in that branch would close it, because copyOf gives each request a complete copy. A follow-up issue is fine if you'd rather keep this PR's scope.

Minor: per-realm credentials. Once the shared cache is filled, RequestAuthCache looks the credentials up with AuthScope(host, ANY_REALM, "basic"). A customizer that registers credentials per realm on the same host and port gets whichever entry BasicCredentialsProvider's best-match scan meets first. If it is the wrong one, the 401 to a complete preemptive scheme ends as FAILURE without a retry. Before this PR, the challenge carried the realm and the right credentials were picked. The default customizer registers a single AuthScope(host, port) and is not affected. Maybe worth a line in the behaviour-change list, together with the opt-out (builder.disableAuthCaching(), or an own AuthCache in beforeRequest).

[maximthomas]

Both points hold; I checked them against the httpclient-osgi 4.3.5 and httpasyncclient-osgi 4.0.2 bytecode.

BASIC_PREEMPTIVE: fixed in this PR (4d8eb37). The default customizer now fills a SharedBasicAuthCache with its new BasicScheme(), so each request gets its own copy, and the remove/put from authFailed/authSucceeded are synchronized. ScriptedCRESTConfigurationTest#testPreemptiveBasicGivesEachRequestItsOwnScheme is red at c58eb0d (expected not same [BASIC] but found [BASIC]).

Pinning it end to end exposed a second gap, identical at 78c1e16: execute ran beforeRequest before getHttpAsyncClient(), so on a client's first request init had not filled the propertyBag yet. That request went out without the preemptive AuthCache (and without the customizer's cookie store) and took a 401. fdde3ca builds the client first. ScriptedCRESTConnectorTest#testPreemptiveBasicNeverTakesA401 is red at c58eb0d and at 4d8eb37 (expected [0] but found [1]). It also fails when the null == localContext.getAuthCache() guard is removed, so the guard is no longer in the not-pinned list.

Per-realm credentials: confirmed. RequestAuthCache.doPreemptiveAuth looks up AuthScope(host, ANY_REALM, "basic"), BasicCredentialsProvider returns the first best match it meets, and after a 401 BasicScheme.processChallenge marks the scheme complete, so HttpAuthenticator ends in FAILURE. I added it to the behaviour-change list with both opt-outs. HttpAsyncClientBuilder.disableAuthCaching() exists in 4.0.2 and drops RequestAuthCache. No code change: carrying the realm in the copy would not help, because the preemptive lookup ignores it.

Description updated (behaviour changes, two test-strength rows, not-pinned list). mvn -pl OpenICF-groovy-connector test: Tests run: 175, Failures: 0, Errors: 0, Skipped: 42.

[vharseko]

Both points are closed.

• BASIC_PREEMPTIVE: 4d8eb37 fills a SharedBasicAuthCache, so each request gets its own BasicScheme, and fdde3ca builds the client before beforeRequest, so a client's first request no longer goes out without the preemptive cache. I ran both new tests against mutants on fdde3ca: reverting the order fails only testPreemptiveBasicNeverTakesA401 (expected [0] but found [1]), a plain BasicAuthCache fails only testPreemptiveBasicGivesEachRequestItsOwnScheme (expected not same), if (true) for the guard fails testPreemptiveBasicNeverTakesA401, and both reverts together fail both. The head is green.
• Per-realm credentials: the behaviour-change line and both opt-outs match httpasyncclient 4.0.2 (disableAuthCaching() drops RequestAuthCache). Agreed that carrying the realm in the copy would not help.

Thanks for catching the init/beforeRequest order as well.