SSE Defects

[jimbogithub]

Describe the bug

Multiple issues related to Server Sent Events demonstrated in https://github.com/jimbogithub/sse. It contains reactive and non-reactive versions of a simple TimeBroadcaster broadcasting a timestamp event once per second to each subscribed TimeConsumer.

SSE Server

See sse-server TimeBroadcaster.

1. Unable to obtain a centralised (e.g. @ApplicationScoped or @Singleton) Sse instance from which to derive the SseBroadcaster and Builder. Hence it appears impossible to properly implement the one-sender-many-receivers pattern.
2. SseBroadcaster.onError is called twice for a client that has been cleanly closed. This is undesirable.

SSE Client

See sse-client TimeConsumer.

1. Requires the Dummy interface in order for the client to bootstrap. See comments on that class as to what happens if you don't have this.

SSE Server Reactive

See sse-server-reactive TimeBroadcaster. Unlike the non-reactive server, it is possible to obtain a singular Sse instance from which to derive the SseBroadcaster and Builder.

1. SseBroadcaster.broadcast tries to send messages to clients that have already closed.
2. SseBroadcaster.onClose is never called.

SSE Client Reactive

See sse-client-reactive TimeConsumer.

1. ClientBuilder.newBuilder()...build() does not work unless you add .withConfig(new org.jboss.resteasy.reactive.common.jaxrs.ConfigurationImpl(RuntimeType.CLIENT)). This wasn't necessary for the non-reactive client or when doing ClientBuilder.newClient() and I suspect is not intended.
2. ClientRequestContext.getHeaders().toString() is not useful and does not match the non-reactive. See further discussion in code.
3. ClientResponseContext.getStatusInfo().toString() is not useful and does not match the non-reactive. See further discussion in code.
4. Client does not send the expected Accept: text/event-stream header.

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

https://github.com/jimbogithub/sse

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.7.2

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

No response

Additional information

Initial discussion: https://groups.google.com/g/quarkus-dev/c/qbH9RYnrIPg/m/GUlEiwLABAAJ

[michalszynkiewicz]

for building the client in the reactive version, is there any reason for not using ClientBuilder.newClient().register(new ClientRequestLogger()).register(new ClientResponseLogger()); to create the client?

[michalszynkiewicz]

CC @FroMage @geoand

[jimbogithub]

for building the client in the reactive version, is there any reason for not using ClientBuilder.newClient().register(new ClientRequestLogger()).register(new ClientResponseLogger()); to create the client?

In my case no. I simply point out the issue that occurs for anyone who does need to access the methods that are only available on the builder.

[jimbogithub]

for building the client in the reactive version, is there any reason for not using ClientBuilder.newClient().register(new ClientRequestLogger()).register(new ClientResponseLogger()); to create the client?

In my case no. I simply point out the issue that occurs for anyone who does need to access the methods that are only available on the builder.

Looks like this one has been addressed by #23949 / #23996.

[jimbogithub]

Another issue with the SSE Client Reactive is that it does not propagate the entity in the WebApplicationException. I think this is a known issue as there are comments in the code to that effect.

This is a more serious issue as I was hoping to switch to the reactive client as it properly implements connection-ttl as documented in https://quarkus.io/guides/all-config#quarkus-rest-client-config_quarkus.rest-client.-config-key-.connection-ttl (idle TTL) whereas the non-reactive version does a hard TTL.

[quarkus-bot]

/cc @FroMage, @geoand, @stuartwdouglas

[herasimau]

Hi, any update on this issue ? SSE Server Reactive
SseBroadcaster.onClose is never called.

[gsmet]

@geoand do you have this one on your radar?

[geoand]

Nope, it had dropped off, but I guess it's back on

[jimbogithub]

FYI, I switched to Mutiny SSE as SSE Client Reactive started rapidly leaking memory (from 2.9.x?) - appears Buffers not being closed.

[geoand]

Do you have a memory dump or a sample project that exhibits this behavior?

[jimbogithub]

Do you have a memory dump or a sample project that exhibits this behavior?

See this branch of the reproducer: https://github.com/jimbogithub/sse/tree/mem-leak

Start the sse-server-reactive then the sse-client-reactive.

2.7.x was OK. Noticed it from 2.9.2. Still present in 2.11.3.
Appears to be related to the Jackson integration as it doesn't occur if the messages are plain and the consumer does either event.readData() or event.readData(String.class).

Not so much leaking byte arrays but they grow in size (increasing #bytes):

jcmd 87720 GC.class_histogram
87720:
 num     #instances         #bytes  class name (module)
-------------------------------------------------------
   1:        119875       57961616  [B ([email protected])

Eventually become "humungous" and GC fails.

[geoand]

Thanks

[geoand]

@jimbogithub I was not able to reproduce the issue with your project.

[jimbogithub]

@geoand Did you use the branch I indicated?

[geoand]

I did yes.

This is what memory looked like:

As you see, the Old Generation size is constant and all allocations are in Eden (which also results in all GCs being Young Generation GCs).
This is what one would expect to see from a well behaving GCed system.

[jimbogithub]

What process are you monitoring? That loaded class count does not look plausible to me.

[geoand]

Oh, it's the client that has the leak? I was monitoring the server. Let me check the client as well

[geoand]

I can confirm the client issue.

[geoand]

#27594 seems to fix the issue but I am not yet 100% it's the proper fix

[jimbogithub]

@geoand I've re-tested my assertion that it was related to the Jackson/JSON integration - it isn't. Reproducer works with server sending PLAIN_TEXT and client doing simple event.readData(). So no need to worry about that aspect.

[geoand]

Right, I saw that too when testing my fix

[geoand]

Is this still a problem?

[geoand]

Closing as we did not receive feedback