CXF-7996: TCK Fixes for SSE reconnect state tests

[andymc12]

This should resolve at least one of the failing SSE tests (specifically, defaultWaiting1s_from_standalone). The TCK is very picky about the isOpen method - and the behavior of the SseEventSource is not terribly intuitive. While the "state" of the SseEventSource might be CLOSED or CONNECTING, the TCK still expects that the isOpen method will return true until it has been explicitly closed (via the close method). I adjusted the unit tests to expect this distinction - but also to continue testing for the proper state of the SseEventSource.

[reta]

Calling 2 callbacks at the same time seems to be a problem, either success (onComplete) or failure (onError) is expected, right?

[andymc12]

The spec is very vague about this, and I don't remember if there was a TCK test that tests this or not...

The notion of invoking both callbacks was discussed in JAX-RS spec issue 606. I have coded up a clarification to resolve that issue (still waiting on one more committer to review it) - and calling both callbacks would satisfy my write-up (and the issue's original request).

I think the main reason for calling both callbacks is to notify the client of two different states:

1. an error occurred.
2. this event source will not receive any more events.

[reta]

Sorry for the delay @andymc12 , just went over the discussion threads, indeed the JAX-RS spec is not very clear regarding the callback. As far as I remember, the initial SSE spec draft was based on Reactive Streams (basically by copy/pasting Java 9's Flow & family). The Reactive Streams specs is very clear:

• onError: unrecoverable error & no more events (failed terminal state)
• onComplete: normal completion & no more events (successful terminal state)

Projecting it to SSE:

• onError: consumer is invoked invoked upon a unrecoverable error encountered (failed terminal state)
• onComplete callback is invoked when there are no further events to be received (successful terminal state)

I have checked Jersey, the reference implementation, and it strictly follows this semantics, so does the CXF's current implementation. I would strongly advocate for sticking to the same semantics: it is widely accepted and understood.

But you are very right by raising jakartaee/rest#606 and jakartaee/rest#670, the spec is not clear and leaves the space for different interpretations.

For me as a developer, the expectation would be to receive either onError or onComplete, but not both.

[andymc12]

No problem @reta - I think this is a good discussion, and I don't have any urgent need to merge it (my comments in the mail list were just to call attention that this change might not be included in 3.3.5, but that should be fine).

It would be nice if the JAX-RS spec was consistent with the Flow APIs. The clarification that I put in with jakartaee/rest#670 came about from recommendations from the RestEasy folks - so I assume that they already call both callbacks. I can see value in both approaches.

Since neither approach is specified in the existing JAX-RS (2.X) specs, would you prefer to stick with the more Flow-like behavior, and then try to change the JAX-RS 3.X API clarification to indicate only one callback should be called? The risk with that is that if the JAX-RS spec community wants to leave it as is with 670 then CXF will need to have different behavior for JAX-RS 3.X as it would in JAX-RS 2.1. I'm fine either way.

Thanks again for the thorough review! On a related note - do you have any interest in joining the JAX-RS spec community? I think it would definitely help to have more Apache representation there.

[reta]

Thanks a lot @andymc12. Yes, I would personally prefer to stick with the more Flow-like behavior (we actually do have one of the SSE examples using RxJava 2 under the hood), it's semantics is well understood (now part of JDK even), thank you.

To be honest, I would be happy to join JAX-RS spec community and be more informed of what is coming (unfortunately that only fits into my spare time), thank you!

[andymc12]

The community is in agreement that only one of onError or onComplete should be invoked (depending on if an error occurred or not), so I'm removing the unnecessary call to onComplete in the connect method.

@reta, thanks for the help on this. Please let me know if you see anything else that might need to change.

[reta]

Thanks @andymc12 , LGTM, good changes!

[reta]

👍

[andymc12]

Thanks for the approval - it looks like these changes may be causing some systest failures - some of them seem unrelated, but others are in the SSE bucket - I'll investigate a little more - it's possible I may need to tweak the tests.