Clarify more about SseBrodcaster.onClose() behavior

[jimma]

SseBroadcaster.onClose() has this description:

    /**
     * Register a listener, which will be called when the SSE event output has been closed (either by client closing the
     * connection or by calling {@link SseEventSink#close()} on the server side.
     * <p>
     * This operation is potentially slow, especially if large number of listeners get registered in the broadcaster. The
     * {@code SseBroadcaster} implementation is optimized to efficiently handle small amounts of concurrent listener
     * registrations and removals and large amounts of registered listener notifications.
     *
     * @param onClose consumer taking single parameter, a {@link SseEventSink}, which was closed.
     */
    void onClose(Consumer<SseEventSink> onClose);

If I undestandand correctly, from the javadoc, these register consumers should be called when the SseEventSink is closed if this SseEventSink is in the broadcast target list. Is this api intended to do nothing about SseBroadCaster.close() ? Should these listeners registered in SseBroadcaster care SseEventSink's close event ?

[spericas]

SseBroadcaster.onClose() has this description:

    /**
     * Register a listener, which will be called when the SSE event output has been closed (either by client closing the
     * connection or by calling {@link SseEventSink#close()} on the server side.
     * <p>
     * This operation is potentially slow, especially if large number of listeners get registered in the broadcaster. The
     * {@code SseBroadcaster} implementation is optimized to efficiently handle small amounts of concurrent listener
     * registrations and removals and large amounts of registered listener notifications.
     *
     * @param onClose consumer taking single parameter, a {@link SseEventSink}, which was closed.
     */
    void onClose(Consumer<SseEventSink> onClose);

If I undestandand correctly, from the javadoc, these register consumers should be called when the SseEventSink is closed if this SseEventSink is in the broadcast target list.

Correct.

Is this api intended to do nothing about SseBroadCaster.close() ?

No, there wouldn't necessarily be an SseEventSink to pass to those consumers right?

Should these listeners registered in SseBroadcaster care SseEventSink's close event ?

Are you asking whether this onClose(Consumer<SseEventSink> onClose) method makes any sense at all?

[jimma]

No, there wouldn't necessarily be an SseEventSink to pass to those consumers right?

Ah, right. Thanks.

Are you asking whether this onClose(Consumer<SseEventSink> onClose) method makes any sense at all?

I don't get In which case we need this api to listen SseEventSink's close event.

[spericas]

Are you asking whether this onClose(Consumer<SseEventSink> onClose) method makes any sense at all?

I don't get In which case we need this api to listen SseEventSink's close event.

As you probably noticed, there is also an onError(...). I think the idea is that as you register SseEventSink's into a broadcaster, you let it inform you of any lifecycle events or error conditions of the underlying sinks.

[jimma]

As you probably noticed, there is also an onError(...). I think the idea is that as you register SseEventSink's into a broadcaster, you let it inform you of any lifecycle events or error conditions of the underlying sinks.

Do that means these listeners are intended to do some cleanup work in SseBroadcaster if there is SseEentSink error or close event happens ? When a SseEventSink is closed, we remove it from the registered list?

[spericas]

@jimma Are you asking as a JAX-RS developer or implementor? As a developer, you just get informed about the event, so it's up to you to do something with that info.

[jimma]

@spericas sorry for the late reply. I am asking as implementor.Just to want to know what does implementor should do here to better understand this api's job.

[spericas]

@jimma Implementations need to: (1) Keep track of the registered consumers and (2) Detect error and closing conditions on underlying sinks. One way to do this may be by wrapping sinks during the registration process and intercepting calls.

[NicoNes]

Hey @spericas,

To be sure to understand, do you mean this more like:

1. An "eager" approach

Like in an Observer\Observable pattern, the SseBroadcaster is a kind of observer for every registered SseEventSink. So, any error/close event from those SseEventSink is directly propagated to the SseBroadcaster and handled by the registered Consumers ?

2. A "lazy" approach

Like a kind of callback mechanism to handle any close/error event detected while broadcasting message or closing the SseBroadcaster .

So:

• when SseBroadcaster.broadcast() is invoked and trying to write to the registered SseEventSink throws an Exception:

  • onErrorConsumers are invoked.
  • If the exception is an IllegalStateException or an IOException, it means that the given SseEventSink has been closed respectively by calling SseEventSink.close() on the server side or by client closing the connection , onCloseConsumers are invoked too.

• when SseBroadcaster.close() is invoked and propagate the close to all registered SseEventSink:

  • oncloseConsumers are invoked for every registered SseEventSink.
  • if any Exception is thrown while closing a registered SseEventSink onErrorConsumers are invoked.

IMO both approaches are right and should be allowed. It would imply to clarify the doc a bit about it, but it would not imply no change for vendors.

WDYT ?

[andymc12]

I noticed that I lost track of a lot of emails a few months back and just recently found them again. Apologies for the slow replies.

IIUC, both approaches are valid, though I tend to prefer the lazy approach. For many implementations, it might not be possible (or at least not cheap and easy) to determine the health of a sink until it attempts to send an event to it - or until the broadcaster is closed. I view the onClose method as intended for the server side to let the clients know that they won't be receiving any more events or maybe to do some cleanup of resources associated with sinks.

@NicoNes do you have a proposal for the clarification?

[spericas]

@NicoNes Sorry for the really late response. I'm not sure the spec really needs to distinguish between these approaches. Is it really worth enforcing implementations to decide one way or the other? Sometimes it is better to engineer some leeway in the process.

I would expect most implementations to use what you call the "lazy" approach, the alternative is likely difficult and costly to implement.

[NicoNes]

Hi @andymc12 and @spericas,

Thanks for the response.

I agree with both of you guys and I also prefer the "lazy" approach that is easier to implement

Is it really worth enforcing implementations to decide one way or the other? Sometimes it is better to engineer some leeway in the process

While implementating SSEBroadcaster spec on RESTEasy I choosed the "lazy" approach that seems to be more obvious/simple to me in respect of the spec. But it raised few questions in the team with people wondering if this "lazy" approach instead of the "eager" one was spec compliant (https://issues.redhat.com/browse/RESTEASY-1819?focusedCommentId=13666490&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13666490). So that why this issue has been opened for clarification.

Now even though this seems obvious for us, it appears to be not so clear for every one. So maybe we could change the SseBroadcaster doc simply like that:

• SseBroadcaster.onError(...):

Register a listener, which will be called when an exception was thrown by a given SseEventSink when this SseBroadcaster tries write to it or close it.

• SseBroadcaster.onClose(...):

Register a listener, which will be called when this SseBroadcaster closes a given SseEventSink or tries to write to a given SseEventSink already closed (either by client closing the connection or by calling {@link SseEventSink#close()} on the server side).

WDYT ?

[andymc12]

@NicoNes, I like that clarification - I would change the wording just slightly to be (my change in bold):

• SseBroadcaster.onError(...):

Register a listener, which will be called when an exception ~~was~~ is thrown by a given SseEventSink when this SseBroadcaster tries to write to it or close it.

• SseBroadcaster.onClose(...)

Register a listener, which will be called when this SseBroadcaster closes a given SseEventSink or tries to write to a given SseEventSink that is already closed (either by client closing the connection or by calling {@link SseEventSink#close()} on the server side).

@spericas (and others) Are you ok with this change for 3.0?

[spericas]

@andymc12 As I said above, I'm not really sure we need these clarifications, but I'm not opposed to making them.

[andymc12]

Thanks @spericas - @NicoNes, do you want to open a PR for these changes? If not, I might be able to do it later next week.

[spericas]

@NicoNes Since you've been working on this, I think you should open it whenever you can.

[NicoNes]

@spericas , I agree with you. @andymc12 Don't bother about it, I will do it at the end of the week or the begining of the next one.

Thanks guys

[NicoNes]

Hey @andymc12 @spericas

Finally I got time to do the PR, here it is https://github.com/eclipse-ee4j/jaxrs-api/pull/887.

[NicoNes]

@jimma can we close this one ?

[jimma]

@NicoNes +1 to close this.

[NicoNes]

@jimma I can't do it cause I'm not the author. Can you do it please ?

Thanks

[spericas]

@NicoNes Let's merge the PR and then I can close this one.

[NicoNes]

Oh you are right the PR is not merged yet. Let's wait.

Thanks