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 https://github.com/spring-projects/spring-framework/issues/31256

Comment From: 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.

Comment From: 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!

Comment From: 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?

Comment From: 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.

Comment From: 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 πŸ˜… )

Comment From: 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! πŸ˜„

Comment From: injae-kim

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

Comment From: 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.

Comment From: injae-kim

https://github.com/spring-projects/spring-framework/blob/fdf187ec465da1d8cc04a8f1b4b892a32f346e33/spring-r2dbc/src/main/java/org/springframework/r2dbc/core/ConnectionAccessor.java#L43-L47

/**
 * 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.

Comment From: 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!

Comment From: 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 πŸ™‡