Introduce ContextAwareRunnable|Callable|Function|Consume|BiConsumer|BiFunction

[vkostyukov]

Motivation:

Users of Armeria APIs might need to be able to query the underlying RequestContext attached to a given "context-aware" Runnable or Callable. Let's name these anonymous classes and give then structure.

Modifications:

• Introduce two new interfaces: ContextAwareRunnable, ContextAwareCallable
• Introduce two new implementations: DefaultContextAwareRunnable, DefaultContextAwareCallable

Result:

• Closes Add ContextAwareRunnable, ContextAwareCallable and such #4878

[vkostyukov]

Changed the return type following the principle of "return the most specialized; accept the most generic".

[anuraaga]

This seems like a breaking change even with API, not just ABI, something like Callable aware = ctx.makeContextAware(notAware); wouldn't compile anymore. Sorry was being silly - but is it confirmed to be ABI compatible?

I guess callers would just need to stick with a type-cast until a major version bump could change the signature, or change the target milestone of the PR to v2.0.0

[vkostyukov]

I think you're right @anuraaga - this could break binary compatibility.

Let's change it back to play it safe?

[anuraaga]

Thanks yeah it seems safer to do that.

Just as an idea, static methods on RequestContext would be an alternative to exposing new types

interface RequestContext {

static RequestContext from(Callable c)
static Callable unwrap(Callable c)
static RequestContext from(Runnable)
static Runnable unwrap(Runnable)
// etc

Naturally it seems like it could get unwieldy if adding even more methods, but if the expectation for that is low it could be viable, just an idea

[vkostyukov]

That's sound idea to structure this. I think we might actually just wait until we can break the binary compatibility. Type-casting isn't too bad and is something I was going to do anyway as the place where I need context() gives me only Runnable.

[ikhoon]

It breaks binary compatibility. We had a similar discussion before. If a change works well with recompilation without modification of users' code, we allowed the change in minor upgrade.

ref: #4760

Simply recompiling your code should be enough in most cases because
BlockingTaskExecutor is a ScheduledExecutorService.

[anuraaga]

Hmm - I have a server framework that uses Armeria that I haven't updated in a while as no new features added. It was nice that Armeria could still be updated in apps using it, and makeContextAware is definitely used so will break. Didn't look at the link maybe it was already broken though 😅 Not a good thing to do in Java since usually you don't compile all your code.

[ikhoon]

I can't say that we strictly follow semantic versioning. Agree on minimizing making breaking changes and maintaining API and ABI compatibility where possible.

That said, if we stick with semantic versioning, we'll have to keep putting things off and Armeria won't evolve easily until the next major version.

It is necessary making the current implicit versioning rule clearer and specify what can happen in a minor upgrade. For example, in the case of Spring Boot, when they release a new version, the previously deprecated functions in two minor versions ago are deleted.

[trustin]

Strictly following semantic versioning will make us get to Armeria 9.0 fairly soon. 😄 We already introduced various ABI-breaking changes similar to this in 1.x unfortunately. Let's keep this PR as it is. Let us have some internal discussion on how we could handle this in our future releases.

[ikhoon]

Thanks for the quick fix! 👍

[jrhee17]

Left some super minor nits. Thanks @vkostyukov 🙇 👍 🚀

[ikhoon]

Still LGTM.

[anuraaga]

This adds two public APIs for doing the same thing I think

ContextAwareBiConsumer.of(ctx, action)
ctx.makeContextAware(action)

Probably it's better to avoid these factory methods since the context has them already

[vkostyukov]

I was mostly following the pattern that's already in place with things like ContextAwareExecutor.

Don't feel strongly about it either way but since we can't (yet) change the type returned from ctx.makeX, maybe leaving another API (ContextAwareBiConsumer) around isn't a bad idea to help users avoid type casting.

[anuraaga]

And another idea as another alternative to the static methods could be a common interface for context aware callbacks

interface ContextAwareCallback<T> {
  RequestContext context();
  T withoutContext();
}

class DefaultContextAwareBiConsumer<T, U> implements BiConsumer<T, U>, ContextAwareCallback<BiConsumer<T, U>> {
}

In the meantime if the caller needs to type cast, they would only have one type to worry about.

[vkostyukov]

I'd actually imagine ContextHolder would be a popular type-cast. At least, that's what I'm going to do internally.

[trustin]

Excellent work, @vkostyukov!