Run response immediate processing update

[injectives]

This update makes run methods (session, tx) wait for successful request acknowledgement by the server. Specifically, the Result object will only be returned when there is a successful response from the server to the RUN message, meaning that the request was at least accepted by the server for processing. Otherwise, an appropriate error will be thrown.

Additionally, this update also makes the Result.keys method non-blocking and free from potential communication errors.

[michael-simons]

From what I understand it seems to make sense. Looks at least simpler in a positive way.
The core idea is not to pass that boolean to ResultCursorFactoryImpl and a new runFuture, right?
Could you elaborate a bit more on the purpose of that runFucture? By maybe choosing a different name?

[injectives]

The core idea is not to pass that boolean to ResultCursorFactoryImpl and a new runFuture, right?

Basically, yes. The waitForRunResponse did not make sense anymore given the goal of these changes. From API perspective, the main objective is to improve the run methods handling by making sure we only return Result object when server successfully accepts the RUN Bolt message (details). In such way the Result object represents a successful acceptance of the request by the server. Otherwise, an error must be thrown.
Previously, it would return Result object in most cases, even if waitForRunResponse was set to true. For instance, see the previous implementation. Keep in mind that runFuture() used to return CompletableFuture<Throwable>.
The new runFuture if part of achieving the aforementioned goal.

[injectives]

Could you elaborate a bit more on the purpose of that runFucture? By maybe choosing a different name?

Basic ResponseHandler implementations take CompletableFuture (or something similar) that is meant to contain the processed response. They are responsible for completing it when appropriate. For instance:

• beginTxFuture in (BeginTxResponseHandler)[https://github.com/neo4j/neo4j-java-driver/blob/4.3/driver/src/main/java/org/neo4j/driver/internal/handlers/BeginTxResponseHandler.java#L32]
• commitFuture in (CommitTxResponseHandler)[https://github.com/neo4j/neo4j-java-driver/blob/4.3/driver/src/main/java/org/neo4j/driver/internal/handlers/CommitTxResponseHandler.java#L34]
• rollbackFuture in (RollbackTxResponseHandler)[https://github.com/neo4j/neo4j-java-driver/blob/4.3/driver/src/main/java/org/neo4j/driver/internal/handlers/RollbackTxResponseHandler.java#L32]
• completionFuture in (ResetResponseHandler)[https://github.com/neo4j/neo4j-java-driver/blob/4.3/driver/src/main/java/org/neo4j/driver/internal/handlers/ResetResponseHandler.java#L31]
• completableFuture in (RouteMessageResponseHandler)[https://github.com/neo4j/neo4j-java-driver/blob/4.3/driver/src/main/java/org/neo4j/driver/internal/handlers/RouteMessageResponseHandler.java#L38]
• etc.

Following this approach, the runFuture is meant to contain the processed response. I am happy to rename it to something else, I just though it could be better to keep the naming similar to other handlers in this change. While there is actually some discrepancy in names, this 'action'Future pattern seems to be a fairly common in the current implementation.

How about introducing doing a separate pull request and introducing a new common naming, like 'action`CompletionFuture (subject to discussion in that PR)? It might be easier from review standpoint.

[michael-simons]

Following this approach, the runFuture is meant to contain the processed response. I am happy to rename it to something else, I just though it could be better to keep the naming similar to other handlers in this change. While there is actually some discrepancy in names, this 'action'Future pattern seems to be a fairly common in the current implementation.

I think responseFuture would fit than.

[injectives]

Following this approach, the runFuture is meant to contain the processed response. I am happy to rename it to something else, I just though it could be better to keep the naming similar to other handlers in this change. While there is actually some discrepancy in names, this 'action'Future pattern seems to be a fairly common in the current implementation.

I think responseFuture would fit than.

As discussed, a separate PR will be created with renames to make them easier to review.

[michael-simons]

Thanks for the excellent walkthrough. I have tested the change afterwards with

• Neo4j-OGM 3.2.25-SNAPSHOT (no failures)
• Cypher-DSL main (no failures)
• Spring Data Neo4j 6.0.10-SNAPSHOT (one failure)

The failure was caused indirectly by our autoclosable query runner as it swallowed the target exception (see change here spring-projects/spring-data-neo4j@dcd422d).

We wouldn't have noticed the change in behavior otherwise, as we only consume via

try {
    var result = s.run();
    result.consume(); // or list, whatever
} catch(Whatever e) {
}

So in any case an exception would be handled.

Having said that, I would agree bringing this into the next patch.