Added a new listener for events related to Client

[neenapj]

No description provided.

[jamezp]

I think the ClientListener should be an interface with default no-op methods. Something like:

public interface ClientListener {
    
    default void connected(final Client client) {
         // no-op
    }
    
    default void closed(final Client client) {
         // no-op
    }

    default void connectionFailed(final Client client, final Throwable cause) {
         // no-op
    }

    default void reconnecting(final Client client, final Throwable cause) {
         // no-op
    }
}

We don't really want to use an enum because it can make it migrations more difficult. Using an interface with no-ops by default allows implementors to implement what they want and not fail if a new enum is not handled.

[mkarg]

I do not see that these events have anything to do with REST, but more with the underlying HTTP or TCP layers (possibly what you want to reach is already feasible by simple registering lower-level event notification?). It might even be impossible for for some implementations to correctly forward these events as their underlying networking framework might possibly not publish these events (for example, Jersey provides an implementation based on JRE's HTTP client, and IMHO that one is unable to fire those events). Unless we are are sure that we actually want to have lower-level functionality mixed into this application-level API, and unless we are sure that all (or at least most) existing compliant implementations are technically able to forward all of these events, we should not adopt this proposal.

[jamezp]

To be fair, the client itself is not really REST related at all. It's a simple HTTP/TCP layer. This solves #1283 which I'd originally created because of an issue I was having with knowing when a client was closed via a CDI producer.

FWIW I'm okay with only having the close notification as that is what I recall needing. I wish I could remember why, but I'm not remembering off the top of my head :)

FWIW there are other things in the JRE's HTTP client that can't be supported either. For example setting a HostnameVerifier on the ClientBuilder.

[neenapj]

Hi @jamezp as per the feedbacks above, shall I remove connected(), connectionFailed() and reconnecting() from ClientListener and keep only closed(Client client) which is universally implementable?

[jamezp]

Hi @jamezp as per the feedbacks above, shall I remove connected(), connectionFailed() and reconnecting() from ClientListener and keep only closed(Client client) which is universally implementable?

Hi @neenapj I think we keep it for now unless @mkarg thinks we should keep reduce the scope.

[mkarg]

I would say remove everything not needed, as Jersey cannot implement the other events besides closed.

[jamezp]

That's good to know, thank you! Really, a close event notification is all I really needed. The others we thought might be useful. However, if they're too difficult to implement we can skip them. We can always consider adding them later.

@neenapj I'd suggest we delete everything but close for now.

[neenapj]

HI @jamezp deleted everything else except close

[jamezp]

This looks good to me. My only preference would be that we squash the commits down to a single commit.

[mkarg]

TCK needed

[neenapj]

Hi @mkarg Tck added and the comments are updated.

[neenapj]

Hi @jamezp

This looks good to me. My only preference would be that we squash the commits down to a single commit.

Do you want me to close this PR and create a new with single commit?

[jamezp]

Hi @jamezp

This looks good to me. My only preference would be that we squash the commits down to a single commit.

Do you want me to close this PR and create a new with single commit?

I would suggest squashing to a single commit and force pushing to your branch. The PR has good comments I'd rather not lose on it.

[neenapj]

Hi @jamezp

This looks good to me. My only preference would be that we squash the commits down to a single commit.

Do you want me to close this PR and create a new with single commit?

I would suggest squashing to a single commit and force pushing to your branch. The PR has good comments I'd rather not lose on it.

I have squashed them into a single commit. Please have a look

[jamezp]

Thank you @neenapj. One minor change needed for the copyright and then I can give it my approval.

[neenapj]

Hi @jamezp changes are done. Kindly review

[mkarg]

@neenapj For any new Jakarta Component Specification Feature there must exist at least one compliant implementation before releasing the spec. Hence, at least one of the current Client implementations MUST proof compliance by passing your new IT. Would you like to provide a PR for the needed changes in Jersey to fulfil this rule?

[jamezp]

I can setup a RESTEasy branch for this, but that shouldn't block merging a feature.

[mkarg]

If we merge it now, it blocks Jakarta REST 5.0 release completely later if no Compliant Implementation exists (due to EF's "Implementation First" policy), so it is easier if any product implements it upfront.

[jamezp]

If we merge it now, it blocks Jakarta REST 5.0 release completely later if no Compliant Implementation exists (due to EF's "Implementation First" policy), so it is easier if any product implements it upfront.

IMO we're already blocked. The only thing that makes a 5.0 worth it is the CDI integration. Otherwise we may as well do a 4.1 if the platform even wants that.

[mkarg]

Let's say, we're delayed, but not blocked. Nothing prevents us from publishing 5.0 even without CDI, and speaking of CDI, I am confident that I will find the time to review your changes in my summer break.

[mkarg]

Seems we can do this in Jersey, see experimental code in eclipse-ee4j/jersey#6121. Waiting for Team Jersey before voting +1 on this new API.