Added close handler on initial response for reactive SseEventSinkImpl #27707

a29340 · 2022-09-04T09:14:12Z

This pr adds the necessary handling of the close event on SseEventSinkIpl. This addresses the scenario when it is the client that is closing the connection, and the sink was not registering itself as closed and most importantly the broadcaster was never notified of the sink close state.

gsmet · 2022-09-19T08:41:33Z

@a29340 no need to rebase for now. @FroMage is on PTO for now. I will ping him when he's back so he can have a look to this one and the other SSE-related one.

a29340 · 2022-09-19T08:45:06Z

@a29340 no need to rebase for now. @FroMage is on PTO for now. I will ping him when he's back so he can have a look to this one and the other SSE-related one.

Oh ok, thank you!

rjvq85 · 2022-09-27T16:44:36Z

.../server/runtime/src/main/java/org/jboss/resteasy/reactive/server/jaxrs/SseEventSinkImpl.java

-            //                // FIXME: notify of client closing
-            //                System.err.println("Server connection closed");
-            //            });
+            response.addCloseHandler(() -> {


Hi!

I checked out your branch and was testing the solution but I cannot understand why the close handler is only added when !response.headWritten(). Is this on purpose? For some reason, on my application this condition is always false and because of that the Close event firing never happens.

Hi @rjvq85, thanks for taking a look!
No, this was not placed there with a specific purpose, I just added the closeHandler where the previous comment was, but it makes sense to move it out of the if statement, to always add it to the response.

For me the condition !response.headWritten() was always true when testing, so it always execute the close handler, and I cannot reproduce a case where it gets evaluated to false... I used the reproducer provided by #23997, with the sse-server-reactive module and it worked fine, the issues on the reactive server seemed to be resolved.

I tested again after moving the changes out of the if statement, and I don't see any unexpected behaviour, so i guess it's fine like that?

FroMage · 2022-10-05T08:33:58Z

.../server/runtime/src/main/java/org/jboss/resteasy/reactive/server/jaxrs/SseEventSinkImpl.java

        }
+        response.addCloseHandler(() -> {
+            if (broadcaster != null) {


I don't think you should do this if block since it's a part of close() which you call anyway. Just call close() here.

That works if the server is the one that closes the connection, the isClosed() returns false so the execution continues and broadcaster.fireClose(this) at line 59 is actually called. When the client is the one that drops, isClosed() method already returns true when the closeHandler is executed and the broadcaster is never notified.
At least this was the case using the reproducer here (using reactive-server)

FroMage · 2022-10-05T08:34:46Z

...erver/runtime/src/main/java/org/jboss/resteasy/reactive/server/jaxrs/SseBroadcasterImpl.java

@@ -72,7 +72,9 @@ public synchronized void close() {

    synchronized void fireClose(SseEventSinkImpl sseEventSink) {
        for (Consumer<SseEventSink> listener : onCloseListeners) {
-            listener.accept(sseEventSink);
+            if (sinks.remove(sseEventSink)) {


Why are you removing the sink from the sinks? This will only match for the first listener, and subsequent listeners won't get called.

I don't get what do you mean, maybe I'm missing something. If we don't remove the sink (which is closing) from the broadcaster, that sink will continue to receive events even if it is closed, right?

You have n listeners. On the first one, you remove the sink, then it's removed, and for all remaining listeners the sink is already removed so you don't notify them. That's a logic bug, no?

Oh, now I see it, sorry! You're right, the removal should happen after all the listeners have been notified. I'll convert this pr to draft and work some more on it, since I'm already adding some tests.

FroMage · 2022-10-05T08:36:07Z

Also, could you add a test for this fix?

a29340 · 2022-11-07T10:06:50Z

Hi @FroMage,

I added mockito as a dependency to do some tests, is it ok?

FroMage · 2022-11-22T10:33:14Z

Thanks. I'm generally in favour of real HTTP tests rather than Mockito. Do you think you could write a RestAssured test that exhibits the issue you're fixing?

gsmet · 2023-02-01T13:53:07Z

@a29340 any chance you could rebase and finalize this?

I don't think we have fixed this issue already?

a29340 · 2023-02-02T08:15:44Z

@gsmet Yes, I will try to finalize this in the next few days. Sorry, I didn´t have a chance to work on it recently

github-actions · 2023-02-05T00:08:59Z

🎊 PR Preview 89fcdc9 has been successfully built and deployed to https://quarkus-io-pr-main-27707-preview.surge.sh/version/main/guides/

FroMage

LGTM, could you rebase on the latest main?

a29340 · 2023-02-14T07:55:51Z

I'm moving this to draft again, some tests are still unstable (often they fail) when run on the CI pipeline on the forked branch.

a29340 · 2023-07-20T23:50:12Z

Hi @FroMage, sorry for the long wait. I finally did fix up the tests covering this PR, and rebased on the latest main and squashed in a single commit

a29340 · 2023-08-08T07:35:48Z

Hi @gsmet @FroMage, is this PR still desired? Should I rebase again waiting for a review?

FroMage · 2023-08-14T13:43:45Z

Hi @gsmet @FroMage, is this PR still desired? Should I rebase again waiting for a review?

Yes please. Sorry about the delays :(

FroMage

Sorry for the late review. This is OK, except the byte-buddy dependency that should be aligned with the Quarkus BOM.

independent-projects/resteasy-reactive/pom.xml

FroMage · 2023-10-10T07:40:47Z

The first test failure seems related to this fix, no?

a29340 · 2023-10-10T07:56:30Z

The first test failure seems related to this fix, no?

Yes, looks like the test didn't detect the expected call on close method on the jdk 20 build. I'm trying to replicate on a local environment, but the tests pass every time. On the forked repo the jdk 11 build failed instead, with the same reason as this build, but passed on the 2nd attempt. Looks like it's not related to the specific java version.
I'll try to make this test more stable and run again a couple of times on the fork before requesting a build on here too

FroMage

Temporarily asking for changes, just to make sure we don't merge this before @gsmet and @geoand answer the dependency question. But my review remains OK otherwise.

FroMage · 2024-01-04T14:29:18Z

independent-projects/resteasy-reactive/pom.xml

+                <type>pom</type>
+                <scope>import</scope>
+            </dependency>
+


Just want to double check with @geoand and @gsmet that they're OK with this dependency being added.

Do we really need the BOM? Isn't adding the mockito test dependency enough?

actually no, let me remove it. I'll try tu run the ci on the fork and then rebase

a29340 · 2024-01-05T09:43:11Z

The first test failure seems related to this fix, no?

Yes, looks like the test didn't detect the expected call on close method on the jdk 20 build. I'm trying to replicate on a local environment, but the tests pass every time. On the forked repo the jdk 11 build failed instead, with the same reason as this build, but passed on the 2nd attempt. Looks like it's not related to the specific java version. I'll try to make this test more stable and run again a couple of times on the fork before requesting a build on here too

Hi @FroMage, I tried to replicate the same test failure on the fork ci for a while now, but couldn't find what was the issue. As of now the tests pass every time I run the ci on the fork. Can you please take a look at the tests in SseServerTestCase to double check?

removed bom for mockito test dependency sorted imports removed colors and added logger instead of system out println formatted added more logs added more logs added more logs, updated bytebuddy formatted adding details to test log

quarkus-bot · 2024-01-10T17:23:47Z

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

FroMage

So, all tests pass, you figured it out?

a29340 · 2024-01-11T10:25:43Z

So, all tests pass, you figured it out?

@FroMage No, the only thing I did was rebase and add logs, but for the last month or so I couldn't replicate the test failure. As of now the tests pass reliably, but I really don't know why they didn't before

FroMage · 2024-01-12T10:19:33Z

Well, we'll see if this occurs again.

a29340 marked this pull request as ready for review September 4, 2022 09:16

geoand requested a review from FroMage September 5, 2022 06:10

gsmet assigned FroMage Sep 5, 2022

gsmet added the triage/backport? label Sep 12, 2022

This comment has been minimized.

Sign in to view

rjvq85 reviewed Sep 27, 2022

View reviewed changes

FroMage requested changes Oct 5, 2022

View reviewed changes

a29340 marked this pull request as draft October 10, 2022 14:31

a29340 marked this pull request as ready for review October 28, 2022 10:16

gsmet removed the triage/backport-2.12? label Nov 17, 2022

This comment has been minimized.

Sign in to view

gsmet added the triage/needs-feedback We are waiting for feedback. label Feb 1, 2023

gsmet added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Feb 4, 2023

quarkus-bot bot added the area/amazon-lambda label Feb 4, 2023

This comment has been minimized.

Sign in to view

a29340 requested a review from FroMage February 5, 2023 09:06

FroMage requested changes Feb 7, 2023

View reviewed changes

a29340 requested a review from FroMage February 7, 2023 15:03

a29340 marked this pull request as draft February 14, 2023 07:56

a29340 marked this pull request as ready for review July 20, 2023 23:46

FroMage requested changes Oct 9, 2023

View reviewed changes

independent-projects/resteasy-reactive/pom.xml Outdated Show resolved Hide resolved

FroMage removed the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Oct 9, 2023

a29340 requested a review from FroMage October 9, 2023 11:37

FroMage approved these changes Oct 9, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

FroMage requested changes Jan 4, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

a29340 added 3 commits January 10, 2024 07:58

addressing resteasy reactive SseBroadcaster issues

f776139

updated mockito and bytebuddy to support java 20

747ce71

updated mockito version

d971e26

removed bom for mockito test dependency sorted imports removed colors and added logger instead of system out println formatted added more logs added more logs added more logs, updated bytebuddy formatted adding details to test log

geoand requested a review from FroMage January 10, 2024 17:25

FroMage approved these changes Jan 11, 2024

View reviewed changes

FroMage merged commit 20eeab4 into quarkusio:main Jan 11, 2024
43 checks passed

quarkus-bot bot added this to the 3.7 - main milestone Jan 11, 2024

Added close handler on initial response for reactive SseEventSinkImpl #27707

Added close handler on initial response for reactive SseEventSinkImpl #27707

Conversation

a29340 commented Sep 4, 2022 • edited Loading

gsmet commented Sep 19, 2022

a29340 commented Sep 19, 2022

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FroMage commented Oct 5, 2022

a29340 commented Nov 7, 2022

FroMage commented Nov 22, 2022

This comment has been minimized.

gsmet commented Feb 1, 2023

a29340 commented Feb 2, 2023

github-actions bot commented Feb 5, 2023

This comment has been minimized.

FroMage left a comment

Choose a reason for hiding this comment

a29340 commented Feb 14, 2023

a29340 commented Jul 20, 2023

a29340 commented Aug 8, 2023

FroMage commented Aug 14, 2023

FroMage left a comment

Choose a reason for hiding this comment

This comment has been minimized.

FroMage commented Oct 10, 2023

a29340 commented Oct 10, 2023

FroMage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

a29340 commented Jan 5, 2024

This comment has been minimized.

quarkus-bot bot commented Jan 10, 2024 • edited by github-actions bot Loading

FroMage left a comment

Choose a reason for hiding this comment

a29340 commented Jan 11, 2024

FroMage commented Jan 12, 2024

a29340 commented Sep 4, 2022 •

edited

Loading

quarkus-bot bot commented Jan 10, 2024 •

edited by github-actions bot

Loading