payara / Payara

Payara Server is an open source middleware platform that supports reliable and secure deployments of Java EE (Jakarta EE) and MicroProfile applications in any environment: on premise, in the cloud or hybrid.
http://www.payara.fish
Other
883 stars 306 forks source link

SseEmitter onError method does not get called on Payara 5 #4227

Closed hsynff closed 4 years ago

hsynff commented 5 years ago

I'm trying below code on Spring MVC (5.1.8 version of dependencies) deployed on Payara 5 :

`@GetMapping("/listen-sse")
public SseEmitter sseEmitter() {
SseEmitter emitter = new SseEmitter(0L);
emitter.onError(e -> System.out.println("OnError"));

ExecutorService service = Executors.newCachedThreadPool();
service.execute(() -> {
    while (true) {
        try {
            Thread.sleep(2000);
            emitter.send("Hello");
        } catch (IOException | InterruptedException e) {
            System.out.println("Exception");
            break;
        }
    }
});

service.shutdown();
return emitter;
}`

When the client disconnects I can only see "Exception" on console but not "OnError". I've tried to deploy same war file to Tomcat 8.5.42 everything works as expected.

rdebusscher commented 5 years ago

Hi @hsynff,

Can you create a small test application so that we can try to reproduce this problem?

Regards Rudy

hsynff commented 5 years ago

Hello. https://github.com/hsynff/sse-test

Kindly, clone this repo.

I'm using below versions: Payara 5.193 Java 1.8_211 (also tried with 1.8_221)

hsynff commented 5 years ago

Hello. Is there any progress? We are going to deploy to production next week, so I have to decide if we must to migrate to the Tomcat or stay in Payara.

OndroMih commented 5 years ago

Hi @hsynff, I tried your reproducer and it gives me exceptions during deployment. Those exceptions are due to a known bug #4060. It's not yet fixed but there's a workaround described in that issue:

Patching jackson-dataformat-xml.jar in the glassfish/modules directory worked as a workaround for us: We changed MANIFEST.MF to allow for stax4 between 3 and 5. It was org.codehaus.stax2;version="[3.1,4)", org.codehaus.stax2.io;version="[3.1,4)",org.codehaus.stax2.ri;version="[3.1,4)" We patched it to be org.codehaus.stax2;version="[3.1,5)", org.codehaus.stax2.io;version="[3.1,5)",org.codehaus.stax2.ri;version="[3.1,5)"

hsynff commented 5 years ago

Yes, I know those exceptions but it's not mandatory for now. Our problem is quite different as I've noted above

OndroMih commented 5 years ago

I debugged the Spring's SseEmitter and I see no way how it triggers the onError method. The emitter just throws an exception, without calling the onError method, there's no code to do that and I'd say it's a bug in Spring. I'll try with Tomcat to see how it's possible that the onError is triggered.

hsynff commented 5 years ago

It works on Tomcat. I've raised same issue on spring issues page and they said that it is related to the container, so there may be a bug in Payara. https://github.com/spring-projects/spring-framework/issues/23668

OndroMih commented 5 years ago

In Tomcat, the onError method is triggered from the Spring's StandardServletAsyncWebRequest, which implements javax.servlet.AsyncListener. I assume that Spring registers the async listener with Tomcat but fails to do so with Payara.

hsynff commented 5 years ago

So is it the issue from Spring or Payara? What should we do for now?

OndroMih commented 5 years ago

I'm really not sure. It looks like Spring registers the async listener but Payara Server doesn't trigger it. It requires more investigation and unfortunately I don't have time for that now.

hsynff commented 5 years ago

Thanks for your time. So I'm gonna close this issue and assume the official feedback from Payara is that "We don't have time for that now" right?

smillidge commented 5 years ago

Yep that's the response from the open source project. You can purchase support for a response with a SLA

smillidge commented 5 years ago

I'll leave open as it seems to be a valid issue

OndroMih commented 5 years ago

To summarize my findings for future investigation:

Payara async context is implemented in AsyncContextImpl

AlanRoth commented 4 years ago

Hi, due to how long ago this issue was raised we have decided to close the issue immediately, and not consider the implementation of the fix/improvement that was requested. Please understand that this decision was taken into consideration with the resources that we have available at the moment. In case of having reported a bug, if the issue is still pressing to you, feel free to verify if it’s applicable in the current release of Payara Community edition, and proceed to raise a new issue with details of the test reproducer. Many thanks for your understanding.