eclipse / microprofile-rest-client

MicroProfile Rest Client
Apache License 2.0
143 stars 72 forks source link

Clarification about closing the rest client #348

Closed moghaddam closed 5 months ago

moghaddam commented 2 years ago

In the Lifecycle of Rest Client section of the spec, it's mentioned that the client should be closed after use. All the examples in the spec are for clients that are being created via builder, not the injected one. So as a reader my assumption is that for automatically injected clients (i.e. via @RestClient) the close method would be called automatically when the client is not in use anymore (e.g. when the CDI bean it's injected into is not used anymore which is the same time that the @PreDestroy callback is called).

Unfortunately seems that's not the case.

It seems that the injected client is not being closed automatically by the container and shortly after it's being used, I get the following in the WildFly log:

WARN [org.jboss.resteasy.client.jaxrs.i18n] (Finalizer) RESTEASY004687: Closing a class org.jboss.resteasy.client.jaxrs.engines.ApacheHttpClient43Engine instance for you. Please close clients yourself.

It's not clear to me if it's a bug in the resteasy implementation or it's an expected behavior based on their interpretation of the spec. So I decided to ask here first what is the expected behavior?.

Do we have to implement the @PreDestroy callback on all such beans to close the clients injected into them?

My test setup is very simple:

The REST client interface:

@RegisterRestClient(baseUri = "https://reqbin.com")
public interface ReqBinAPI extends AutoCloseable {
    @GET
    @Produces(MediaType.APPLICATION_JSON)
    @Path("/echo/get/json")
    String get();
}

The JAX-RS resource that is calling it:

@Path("/test")
public class TestResource {

    @Inject
    @RestClient
    ReqBinAPI reqBinAPI;

    @GET
    public Response method1() {
        String result = "default-value";
        try {
            System.out.println("method1 called");
            result = reqBinAPI.get();
        } catch (Exception e) {
            System.out.println(e.getMessage());
        }
        return Response.ok(result).build();
    }
}

Environment:

andymc12 commented 2 years ago

It looks like the spec is silent on whether/when a CDI-created client should be closed. I agree that the logical point to close one would be when the instance goes out of scope (e.g. at app shutdown for @ApplicationScoped clients, or once the request is completed for @RequestScoped clients).

I think it would be good to clarify that in the spec - and verify an implementation's compliance with a TCK test. I don;t have the bandwidth for those changes, but possibly there are some volunteers?

All that said, it might still not be a functional bug in Wildfly/RESTEasy per se - it is possible that they are still closing the client instance implicitly, but logging the message that the underlying JAX-RS implementation class is not closed. Ideally, they would avoid logging that message, but it is possible that the resources associated with the Rest Client instance are properly cleaned up - you'd need to do further testing to verify that - or discuss with the RedHat team.

Emily-Jiang commented 9 months ago

@jamezp will take a look at it.