Async HTTP

[Aklakan]

GitHub issue resolved #3471

This is the async HTTP part factored out from #3184 .

Pull request Description:

• Update HTTP(s) machinery to use async requests. This allows for QueryExecHTTP.abort() to immediately return even if the server has not yet responded to the connection request.
• Fixes resource leak in Service.java
• Adds (nullable) getUpdateString and getUpdateRequest methods to UpdateProcessor so that the underlying request may be exposed.

---

• [ ] Tests are included (no additional tests added)
• Commits have been squashed to remove intermediate development commit messages.
• Key commit messages start with the issue number (GH-xxxx)

By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement.

---

See the Apache Jena "Contributing" guide.

[rvesse]

I like the idea of making some of the HTTP stuff Async, though I wonder what the knock on effect will be on end users as I know some users in the past, myself included, have used Jena's HTTP helpers directly, so the HttpLib changes in particular may have some unexpected side effects for downstream users.

There seems to be some leftover debugging code around exception handling that looks like it needs tidying up

[Aklakan]

[...] the HttpLib changes in particular may have some unexpected side effects for downstream users.

The main change should be only that the original sync methods now internally wait for the completion of a CompletableFuture. My feeling is that maintaining an additional non-async implementation based on the sync java.net.http API might eventually cause issues as it duplicates code that may diverge. The original test cases pass that way, but I am not sure how well tested this part is. Perhaps your previous use of the API could be added as (integration) tests?

[rvesse]

[...] the HttpLib changes in particular may have some unexpected side effects for downstream users.

The main change should be only that the original sync methods now internally wait for the completion of a CompletableFuture. My feeling is that maintaining an additional non-async implementation based on the sync java.net.http API might eventually cause issues as it duplicates code that may diverge. The original test cases pass that way, but I am not sure how well tested this part is.

Totally agree, and no problems with this general approach

Perhaps your previous use of the API could be added as (integration) tests?

That was many years ago now and against a much older Jena version, the code in question has bit-rotted horribly. These days I use other APIs for HTTP stuff rather than leaning on Jena's APIs.

This might be a non-issue, just something to highlight and make sure we communicate to users in the release this eventually lands in.

[afs]

It is already async!

The JDK implementation of HttpClient.send (HttpClientImpl.send) is a call to HttpClientImpl.sendAsync then CompletableFuture.get with some exception management around that.

General use of HTTP/2 and HTTP/3 makes it necessary because they both multiplex the underlying network connection.

[Aklakan]

It is already async!

Right but that's the JDK internals. For clarification: Jena's HttpLib.executeJDK relied on HttpClient.send and this PR changes this and HttpLib's surrounding logic to HttpClient.sendAsync.

I removed the breadcrumb exceptions (the clean way to retain the breadcrumbs seems to be wrapping with custom checked exceptions).

[afs]

This looks like it is a change to exception handling in the sync case. (I hope I read the code correctly.) Does this matter?

AsyncHttpRDF.getOrElseThrow passes up runtime exceptions untouched. That means the app gets the exception from the async thread and a stacktrace does not indicate where in the application code their synchronous call was made.

HttpClientImpl.send builds an exception with the other threads exception as the cause. The app gets an exception with the app stack. When printed, the stacktrace identifies the app code that makes the sync call.

The HttpClientImpl.send exception rebuild is a bit yuk - it has to enumerate the possible classes.

We could have a HttpException, even HttpAsyncException < HttpException, as a carrier to the app call stack making it robust against any future exception introduced by the JDK library. The change would be here, although in AsyncHttpRDF.getOrElseThrow is a possibility as well.

[Aklakan]

That means the app gets the exception from the async thread and a stacktrace does not indicate where in the application code their synchronous call was made.

Yes, that's the issue.

I think your suggestion to wrap an exception with the already existing unchecked HttpException is the clean way forward. Consolidating this exception handling in AsyncHttpRDF.getOrElseThrow seems reasonable.

A variant of getOrElseThrow with an additional HttpRequest argument could be used to add the request URIs to the exceptions (which is what happens in HttpLib):

throw new HttpException(httpRequest.method()+" "+httpRequest.uri().toString(), cause);

[Aklakan]

Code was revised and is ready for review.

[Aklakan]

I have completed my revisions:

• There is now a unified AsyncHttpRDF.getOrElseThrow(CompletableFuture<T> cf, HttpRequest httpRequest) method that wraps any exception as an HTTPException and in some cases also adds information from the optional httpRequest argument. Existing references were updated to use this method.
• QueryExecHTTP.abort() no longer closes the physical HTTP InputStream but just a wrapper. Only the close() method (assumed to be called from the thread that started the query exec) will close the physical input stream and thereby consume any remaining data.

[Aklakan]

Just as a note: This is still not the nicest solution because it pulls the rug from under the parsers' feet - but the original solution also just closed the physical HTTP input stream.
Now abort() just sets a flag (instant action) that causes ProxyInputStream's next read to fail - whereas only close() closes the physical stream (may take time to consume remaining data).
If really needed, further revisions could go into future PRs.

[afs]

• QueryExecHTTP.abort() no longer closes the physical HTTP InputStream but just a wrapper. Only the close() method (assumed to be called from the thread that started the query exec) will close the physical input stream and thereby consume any remaining data.

As long as someone ™️ does an exhaustive close to the HTTP connection.

[afs]

One conflict otherwise looks good.

[Aklakan]

Conflict resolved.