Subscribing to CompletionStage returned by Event.fireAsync doesn't seem to work correctly

[edeandrea]

Describe the bug

When trying to use javax.enterprise.event.Event.fireAsync, it does not seem to subscribe to the CompletionStage that is returned.

Expected behavior

If I wrap Event.fireAsync inside a Uni.createFrom().completionStage(), I would expect the result of the handler of the event to be subscribed to.

Actual behavior

It does not

How to Reproduce?

Reproducer:
fireAsyncBug.zip

1. Unzip the reproducer
2. Run Quarkus dev mode
3. Turn on continuous testing. You'll see that the EventResourceTests.eventReverses test fails

In EventResource there is a single jax-rs method:

@GET
@Produces(MediaType.TEXT_PLAIN)
public Uni<String> event() {
	return Uni.createFrom().completionStage(
		this.event.fireAsync("Hello")
	);
}

The EventListener class handles the event:

@ApplicationScoped
public class EventListener {
	public Uni<String> onEvent(@ObservesAsync String event) {
		return Uni.createFrom().item(new StringBuilder(event).reverse().toString());
	}
}

The Uni returned by EventListener.onEvent is never subscribed to. This leads me to the question "What actually is the CompletionStage returned by Event.fireAsync? What does it represent?

Output of uname -a or ver

Darwin edeandrea-m1pro 22.1.0 Darwin Kernel Version 22.1.0: Sun Oct  9 20:15:09 PDT 2022; root:xnu-8792.41.9~2/RELEASE_ARM64_T6000 arm64

Output of java -version

openjdk version "17.0.5" 2022-10-18
OpenJDK Runtime Environment Temurin-17.0.5+8 (build 17.0.5+8)
OpenJDK 64-Bit Server VM Temurin-17.0.5+8 (build 17.0.5+8, mixed mode)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.14.0.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
Maven home: /Users/edeandre/.m2/wrapper/dists/apache-maven-3.8.6-bin/67568434/apache-maven-3.8.6
Java version: 17.0.5, vendor: Eclipse Adoptium, runtime: /Users/edeandre/.sdkman/candidates/java/17.0.5-tem
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "13.0", arch: "aarch64", family: "mac"

Additional information

No response

[edeandrea]

Or maybe is my understanding of how fireAsync is supposed to work? Should the observer method still return void? If so, how would that work in the reactive world, where maybe the observer method is interacting with something else reactive (i.e. Hibernate reactive).

Something needs to be able to subscribe to whatever the observer does...In this case should there be some custom subscriber that does something in the observer?

[quarkus-bot]

/cc @manovotn, @mkouba

[mkouba]

Should the observer method still return void?

Yes, CDI observer methods must always return void.

The Uni returned by EventListener.onEvent is never subscribed to. This leads me to the question "What actually is the CompletionStage returned by Event.fireAsync? What does it represent?

The returned CompletionStage is completed when all async observer methods are notified. The payload is the event object, i.e. String in your case.

If so, how would that work in the reactive world, where maybe the observer method is interacting with something else reactive (i.e. Hibernate reactive).

CDI async observers were not designed to participate in a reactive stream. The logic used inside observer method is expected to be synchronous/blocking.

Something needs to be able to subscribe to whatever the observer does...In this case should there be some custom subscriber that does something in the observer?

You can only subscribe to the result of the notification.

[geoand]

@mkouba do we perhaps need to improve the documentation?

[mkouba]

@mkouba do we perhaps need to improve the documentation?

Well, our documentation does not cover the details of standard CDI functionalities. We could try to add something somewhere. But where is the border line, i.e. which features to mention and which not...

[geoand]

My point on this is that it's mixing standard and non-standard behavior, so it's really hard for users to know what exactly to expect. I think a few examples in the documentation would go a long way

[mkouba]

My point on this is that it's mixing standard and non-standard behavior, so it's really hard for users to know what exactly to expect.

Well, most (if not all) non-standard features are described in the docs and everything else should be standard. And this behaves as defined by the spec. It's the user creativity that mixes apples and oranges ;-). But I get your point.

We could add something like "CDI in the reactive world". But it would be basically just an incomplete list of unsupported combinations.

Note that Weld docs has roughly 190 pages and does not mention reactive at all ;-).

[Ladicek]

Even though the CDI specification doesn't seem to require observer methods to declare a return type of void, I believe we should just treat non-void-returning observer methods as a definition error, frankly.

[geoand]

Right, I see your point @mkouba.
Perhaps we should just take the pragmatic approach off adding examples of things that we know are confusing people.

[geoand]

I believe we should just treat non-void-returning observer methods as a definition error, frankly.

Yeah, that's a good idea - making things explicit for users is usually very helpful

[mkouba]

Even though the CDI specification doesn't seem to require observer methods to declare a return type of void, I believe we should just treat non-void-returning observer methods as a definition error, frankly.

Yes, that would make sense. The truth is that the CDI spec implicitly mandates to ignore the return type but it would not hurt to fail the build.

NOTE: Unless there's someone who is using an observer method in a very creative way, i.e. call an observer method directly... which is not forbidden either.

[edeandrea]

Lots of good info on this thread. Thank you all for your comments.

After I created it I kinda felt like the issue was mine in that I wasn't understanding how it should work.

Based on all these great comments my understanding is that observer methods shouldn't be used with reactive and that we should find a different solution for what we're trying to do?

Maybe use the Vert.x EventBus?

In my case the thing firing the event doesn't necessarily have to care about the result of what the observer does, but the observer does do reactive "stuff" (using Hibernate reactive) and therefore something needs to subscribe to it's result. So maybe I need to create a custom subscriber or something.

The example I attached to this thread was a very simplified and dumbed-down example.

[edeandrea]

The returned CompletionStage is completed when all async observer methods are notified.

This is key info! The CompletionStage completes simply when all observers have been notified. Has nothing to do with if they've completed.

The Event class itself isn't part of Quarkus, but would be nice if that info was right there on it's Javadocs!

CDI async observers were not designed to participate in a reactive stream. The logic used inside observer method is expected to be synchronous/blocking.

This is also key info and would be great if it were documented somewhere. Maybe it is and I was just lazy and didn't go looking for it :)

[edeandrea]

@ingmarfjolla lots of good info here...

[mkouba]

Maybe use the Vert.x EventBus?

Yes, you can try the EventBus and @ConsumeEvent. But I wonder if you need events at all. What does your use case look like?

The Event class itself isn't part of Quarkus, but would be nice if that info was right there on it's Javadocs!

That could be difficult. It's part of the CDI API that is not released very often...

This is also key info and would be great if it were documented somewhere. Maybe it is and I was just lazy and didn't go looking for it :)

I don't think it's documented in quarkus. And the reason is that CDI itself is very mature (2009 ;-) and was not designed with reactive in mind. As I said above - we should probably mention it somewhere... :-)

[edeandrea]

The use case actually comes from the Debezium outbox extension. @ingmarfjolla has been working on trying to contribute a "reactive" version.

The way the current blocking implementation works is the consumer creates the Outbox event, and using the Event, does a fire (https://github.com/debezium/debezium/blob/main/debezium-quarkus-outbox/integration-tests/src/main/java/io/debezium/outbox/quarkus/it/MyService.java#L30).

Then the Debezium event dispatcher observes that event and persists it into the outbox table (https://github.com/debezium/debezium/blob/main/debezium-quarkus-outbox/runtime/src/main/java/io/debezium/outbox/quarkus/internal/EventDispatcher.java#L25, https://github.com/debezium/debezium/blob/main/debezium-quarkus-outbox/runtime/src/main/java/io/debezium/outbox/quarkus/internal/DefaultEventDispatcher.java, and https://github.com/debezium/debezium/blob/main/debezium-quarkus-outbox/runtime/src/main/java/io/debezium/outbox/quarkus/internal/DebeziumTracerEventDispatcher.java).

@ingmarfjolla was trying to create a reactive version of this but was trying to use the same pattern, but the problem now is the onExportedEvent needs to be reactive using a reactive session, so it doesn't look like this event notification/subscription architecture is going to work in this case.

Maybe we need to get the debezium team roped into this conversation?

[mkouba]

...needs to be reactive using a reactive session, so it doesn't look like this event notification/subscription architecture is going to work in this case.

So it's going to use hibernate-reactive, right? CC @DavideD

Maybe we need to get the debezium team roped into this conversation?

Plus our reactive team... CC @cescoffier @jponge

[edeandrea]

So it's going to use hibernate-reactive, right?

Yes, using Mutiny.SessionFactory

@ingmarfjolla has a prototype he's been working on (that I've been helping him with): https://github.com/ingmarfjolla/debeziumreactive/tree/quarkus-outbox-reactive-refactor/debezium-quarkus-outbox-reactive

[ingmarfjolla]

we are going to try the https://quarkus.io/guides/vertx-reference#eventbus . Appreciate all the info dropped here I'm learning a lot

[ingmarfjolla]

maybe a silly question, but is there a way when using the event bus to create codecs in the deployment module of an extension and then use them in an application? i saw this but I wasn't sure how to make use of it

[cescoffier]

Why do you need a codec? Quarkus generates one for you automatically. If you need one just add a codec class in your app, and it will pick it for you.

If you are in an extension, just produce the build item you found.

[ingmarfjolla]

For some reason when Quarkus starts I was getting an error that the codec would be generated, and then that quarkus failed to start because it couldn't find the class. here The logs would say `could not find class exportedevent but when I dropped the question mark I end up getting an error in my integration tests because here now since it's saying no codec was created for MyOutboxEvent. So i'm guessing I will have to produce the build item?

[mkouba]

For the record - here is the spec issue about validating the return type of an observer method.

[mkouba]

It seems that there is no consensus yet. So I suggest to log a warning for now. WDYM?

[manovotn]

It seems that there is no consensus yet. So I suggest to log a warning for now. WDYM?

From the issue discussion and from CDI meetings my understanding was that the main reason for why we cannot enforce void return type on spec level is legacy code and some very creative (and very edge) cases.
We could choose to make Quarkus stricter to avoid getting to these creative solutions here as well :)

A one-time warning works too but I'd say if we think its not a good way, we should just throw an error.
WDYT @Ladicek?

[mkouba]

It seems that there is no consensus yet. So I suggest to log a warning for now. WDYM?

From the issue discussion and from CDI meetings my understanding was that the main reason for why we cannot enforce void return type on spec level is legacy code and some very creative (and very edge) cases. We could choose to make Quarkus stricter to avoid getting to these creative solutions here as well :)

A one-time warning works too but I'd say if we think its not a good way, we should just throw an error. WDYT @Ladicek?

My point is that we might want to be spec-compliant in the future (i.e. pass the TCK). And this could result in a TCK-fallback config property... which might be required anyway but it's still not a good thing to do.

[Ladicek]

I don't really have an opinion, frankly. We could maybe treat this as a feature request? We do similar things elsewhere, and it would make sense.

[geoand]

Do we plan on doing anything on this one?

[mkouba]

Do we plan on doing anything on this one?

The spec issue was closed so the validation of the return type is probably not going to be enforced.

So we could probably support observer methods that return Uni. However, it requires further investigation.