Add error handling function in R2DBC DatabaseClient

[injae-kim]

Closes gh-31256

Motivation

// users want to handle error like this on various cases!
    @Override
    public <T> Mono<T> inConnection(Function<Connection, Mono<T>> action) throws DataAccessException {
        return super.inConnection(con -> action.apply(con)
                .onErrorResume(e -> {
                    handleError(e, con); // here
                    return Mono.error(e);
                }));
    }

    @Override
    public <T> Flux<T> inConnectionMany(Function<Connection, Flux<T>> action) throws DataAccessException {
        return super.inConnectionMany(con -> action.apply(con)
                .onErrorResume(e -> {
                    handleError(e, con); // here
                    return Mono.error(e);
                }));
    }

• We found that users want to handle error by user's custom error handling logic on DatabaseClient, but currently impossible

Modification

DatabaseClient.builder()
    .handleInConnectionError((t, conn) -> { /* error handling logic */})
    .build()

• Add error handling function on R2DBC DatabaseClient and it's builder

Result

• Now user can use their custom error handling logic on R2DBC DatabaseClient
• Close Add error handling strategy to R2DBC DatabaseClient #31256

[jhoeller]

Thanks for the pull request! From a quick first glance, this looks pretty good to me :-)

A code-style note: Please do not use star imports such as io.r2dbc.spi.*; we always list all imported classes explicitly. You may run the check build target instead of test, including our Checkstyle task as well which checks for such common code style issues.

A naming note: handleInConnectionError is descriptive but sounds like a protected template method rather than a callback. Maybe onConnectionError with an event-style on prefix instead? The specific semantics of being called for exceptions that come up within the inConnection method could be put into a javadoc, still making it descriptive enough.

[injae-kim]

oh thanks a lot for detailed review @jhoeller !

• Fix import * to import all classes explicitly
• Fix naming handleInConnectionError to onConnectionError
• Run checkStyle and build and check success

✅ I updated PR like above based on your suggestion. thanks!

[sbrannen]

Please rename instances of the BiConsumer to errorHandler (or perhaps connectionErrorHandler).

Thanks

[jhoeller]

@sbrannen @injae-kim indeed, what we register via onConnectionError is an error handler function, terminology-wise, and that should be reflected in the argument name.

@mp911de does the overall arrangement look sensible to you?

[mp911de]

Taking a step back, I wonder what we actually want to achieve. Generally, connections are released (closed) upon an exception and the error signal is dispatched to the subscriber.

DatabaseClient has a concept of functions (ExecuteFunction, StatementFilterFunction) so I'm wondering whether it would rather make sense to have ConnectionFilterFunction that acts as hook for all inConnection[Many] invocations. inConnection was built with the intent to serve as customization hook. Introducing a filter function that can serve this purpose would take the error handler into consideration and provide customization beyond just error handling.

[injae-kim]

Taking a step back, I wonder what we actually want to achieve.

I just simply want to make user can handleError with connection like handleError(t, conn) on inConnection[Many]!
e.g. user can leave log or open circuit breaker with connection info and error by using handleError(t, conn)

Currently it's impossible cause connection is only used inside of inConnection[Many].

I'm wondering whether it would rather make sense to have ConnectionFilterFunction that acts as hook for all inConnection[Many] invocations.

@mp911de , thanks a lot for your suggestion!
Can you share some example code about ConnectionFilterFunction usage?
(it's my first PR on spring-r2dbc so I'm hard to imagine how ConnectionFilterFunction works like 😅 )

[injae-kim]

(@mp911de) Introducing a ConnectionFilterFunction that can serve this purpose would take the error handler into consideration and provide customization beyond just error handling.

Hi @@sbrannen, @jhoeller what do you think about above suggestion?

If you agree with this, please share to me~! I'll update this PR with this way!
(introducing ConnectionFilterFunction instead of BiConsumer<> errorHandler)

I also agree that ConnectionFilterFunction can provide customization beyond just error handling, but for now user may only need BiFunction<> errorHandler and it's simpler. waiting your opinion! 😄

[injae-kim]

Rebase to upstream/main to resolve conflict on test~!

[snicoll]

Looking at the latest state of this PR, I don't really see how the consumer is related to "connection error". The function that uses the connection may also lead to an error, and if closing the connection fails, it is not invoked. Looking at the javadoc of onConnectionError I don't believe it matches what the algorithm does.

[injae-kim]

spring-framework/spring-r2dbc/src/main/java/org/springframework/r2dbc/core/ConnectionAccessor.java

Lines 43 to 47 in fdf187e

	public interface ConnectionAccessor {
	/**
	* Execute a callback {@link Function} within a {@link Connection} scope.
	* The function is responsible for creating a {@link Mono}. The connection

/**
 * Handle an exception thrown from {@link ConnectionAccessor#inConnection}
 * or {@link ConnectionAccessor#inConnectionMany} 👉 (new) during executing a callback {@link Function} within a {@link Connection} scope.
 * @param errorHandler a {@code BiConsumer} that handles the connection error
 * @since 6.2
 */
Builder onConnectionError(BiConsumer<? super Throwable, ? super Connection> errorHandler)

onConnectionError actually consumes error thrown during action.apply(connection).
And "action is a callback function within a connection scope", based on ConnectionAccessor.inConnection* comment.

So it's good to enhance onConnectionError javadoc like above? matching with what it really does.

[snicoll]

So it's good to enhance onConnectionError javadoc like above? matching with what it really does.

I don't think so and I am not sure that is where we want to go anyway. Brainstorming with @mp911de, we'd like to take a step back and resume the conversation on the issue. Specifically by sharing a representative sample of what that would achieve. It's unclear if you're affected by that issue as well or if you're trying to help based on the original report. Either way, let's continue chatting on the issue. Thanks for the PR, in any case!

[injae-kim]

Either way, let's continue chatting on the issue.

aha I got it! let's continue to discuss on issue~ thanks a lot for review 🙇