Exception handler for context aware data structure

[tomatophobia]

Motivation:

• Armeria provides several context-aware data types. However, in the absence of exception handling logic, if an exception occurs, the error is lost and not propagated to the caller.

Modifications:

• An additional RequestContext#makeContextAware method that accepts an exception handler as an argument has been added.
• Context-aware data types has been enhanced to handle exceptions using the exception handler.

Result:

• Closes Exception handler for ContextAwareExecutor #4879
• Users can make their own data types context-aware with a custom exception handler. If an exception occurs, it can be handled with the specified handler.

[tomatophobia]

I have two questions. I would appreciate it if you could provide your opinions. 🙇

I have a concern about makeContextAware(ExecutorService). We cannot know in advance which functions will be submitted to the ExecutorService instance. Therefore, I believe the type of the exception handler also has to be Function<Throwable, Object>. This, in my opinion, limits the use of the type system and can easily lead to runtime errors.

So, for the case of makeContextAware(ExecutorService), instead of separately passing an exception handler as a parameter, I considered adding submit(Callable, Function<Throwable, T>) to ContextAwareExecutorService interface. I would like to know the opinions on this situation.

I deliberately did not add exception handler accessor methods to context-aware classes. Is it necessary?
For example:

@Nullable
Consumer<Throwable> exceptionHandler() {
    return exceptionHandler;
}

[codecov]

Codecov Report

Attention: Patch coverage is 72.89720% with 29 lines in your changes missing coverage. Please review.

Project coverage is 74.29%. Comparing base (863e27c) to head (d4e9ec0).
Report is 586 commits behind head on main.

Files with missing lines	Patch %	Lines
.../armeria/common/DefaultContextAwareBiConsumer.java	60.00%	3 Missing and 1 partial ⚠️
.../armeria/common/DefaultContextAwareBiFunction.java	60.00%	3 Missing and 1 partial ⚠️
...rp/armeria/common/DefaultContextAwareConsumer.java	60.00%	3 Missing and 1 partial ⚠️
...rp/armeria/common/DefaultContextAwareFunction.java	60.00%	3 Missing and 1 partial ⚠️
.../linecorp/armeria/common/ContextAwareExecutor.java	57.14%	2 Missing and 1 partial ⚠️
...p/armeria/common/AbstractContextAwareExecutor.java	80.00%	1 Missing and 1 partial ⚠️
...rp/armeria/common/DefaultContextAwareCallable.java	80.00%	1 Missing and 1 partial ⚠️
...rmeria/common/PropagatingContextAwareExecutor.java	71.42%	1 Missing and 1 partial ⚠️
...inecorp/armeria/common/ContextAwareBiConsumer.java	50.00%	1 Missing ⚠️
...inecorp/armeria/common/ContextAwareBiFunction.java	50.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5164      +/-   ##
============================================
+ Coverage     74.25%   74.29%   +0.04%     
- Complexity    19825    19861      +36     
============================================
  Files          1699     1699              
  Lines         73046    73150     +104     
  Branches       9357     9366       +9     
============================================
+ Hits          54239    54347     +108     
  Misses        14371    14371              
+ Partials       4436     4432       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

🔍 Build Scan® (commit: d4e9ec0)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/kly2ygsi4v3ku
build-self-hosted-jdk-8	✅	https://ge.armeria.dev/s/giwt7phvelz6g
build-self-hosted-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/gy72le76xp7k2
build-self-hosted-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/7dv7wmb2ihr7g
build-self-hosted-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/hdqvwtbv3rbky
build-self-hosted-jdk-17-leak	✅	https://ge.armeria.dev/s/xfuvrha6inv2c
build-self-hosted-jdk-11	✅	https://ge.armeria.dev/s/ow3hhvwor7vd2
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/seh2z646jx2j6

[jrhee17]

So, for the case of makeContextAware(ExecutorService), instead of separately passing an exception handler as a parameter, I considered adding submit(Callable, Function<Throwable, T>) to ContextAwareExecutorService interface. I would like to know the opinions on this situation.

I deliberately did not add exception handler accessor methods to context-aware classes. Is it necessary?
For example:

I think it's out of this pull request's scope to specify exceptions thrown from executors.
My opinion is that this is the responsibility of the executor, not context aware callbacks.

• ThreadFactories.builder("").taskFunction()
• Thread#setDefaultUncaughtExceptionHandler

By this reasoning, I don't think ContextAwareExecutor doesn't need the extra API either.
Any thoughts @trustin @minwoox @ikhoon ?

[ikhoon]

submit(Callable, Function<Throwable, T>) to ContextAwareExecutorService interface. I would like to know the opinions on this situation.

I am not sure if submit(Callable, Function<Throwable, T>) is really useful. A try-catch block would be intuitive and easier rather than specifying a separate exception handler for @FunctionalInterfaces.

// Wrap an action with try-catch block
submit(() -> {
  try {
     // Perform an action
  } catch (e) {
     ...
  }
});

I prefer adding only makeContextAware(Executor executor, Consumer<Throwable> uncaughtExceptionHandler) and makeContextPropagating(Executor executor, Consumer<Throwable> uncaughtExceptionHandler) which is used to catch uncaught exceptions for Runnable tasks.

Note that exceptions raised by Callable are not uncaught exceptions because the exceptions are propagated via the returned Future.