From da35d95a95762eef0f07190e95d69e129a8280c9 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Fri, 21 Feb 2025 19:19:17 -0500 Subject: [PATCH] CXF-8992: WebClient.fromClient() broken due to garbage collection --- .../cxf/jaxrs/client/AbstractClient.java | 85 ++++++++++++++++--- .../apache/cxf/jaxrs/client/WebClient.java | 2 +- .../cxf/jaxrs/client/WebClientTest.java | 33 ++++++- 3 files changed, 107 insertions(+), 13 deletions(-) diff --git a/rt/rs/client/src/main/java/org/apache/cxf/jaxrs/client/AbstractClient.java b/rt/rs/client/src/main/java/org/apache/cxf/jaxrs/client/AbstractClient.java index 3b839638047..6a6a3a700bf 100644 --- a/rt/rs/client/src/main/java/org/apache/cxf/jaxrs/client/AbstractClient.java +++ b/rt/rs/client/src/main/java/org/apache/cxf/jaxrs/client/AbstractClient.java @@ -41,10 +41,12 @@ import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; import java.util.logging.Logger; import javax.xml.stream.XMLStreamWriter; +import jakarta.annotation.Nullable; import jakarta.ws.rs.PathParam; import jakarta.ws.rs.ProcessingException; import jakarta.ws.rs.WebApplicationException; @@ -130,6 +132,34 @@ public abstract class AbstractClient implements Client { protected ClientConfiguration cfg = new ClientConfiguration(); private ClientState state; private final AtomicBoolean closed = new AtomicBoolean(); + private volatile ConfigurationReference configurationReference = new ConfigurationReference(cfg); + + protected static final class ConfigurationReference { + private final AtomicLong refCount = new AtomicLong(1); + private final ClientConfiguration cfg; + + public ConfigurationReference(ClientConfiguration cfg) { + this.cfg = cfg; + } + + public ConfigurationReference acquire() { + refCount.incrementAndGet(); + return this; + } + + public long refCount() { + return refCount.get(); + } + + public long release() { + return refCount.decrementAndGet(); + } + + public ClientConfiguration getConfiguration() { + return cfg; + } + } + protected AbstractClient(ClientState initialState) { this.state = initialState; } @@ -349,7 +379,10 @@ public void close() { if (cfg.getBus() == null) { return; } - cfg.getEndpoint().getCleanupHooks(). + + final boolean shutdownConfiguration = configurationReference.release() == 0L; + if (shutdownConfiguration) { + cfg.getEndpoint().getCleanupHooks(). forEach(c -> { try { c.close(); @@ -357,26 +390,35 @@ public void close() { //ignore } }); - ClientLifeCycleManager mgr = cfg.getBus().getExtension(ClientLifeCycleManager.class); + } + + final ClientLifeCycleManager mgr = cfg.getBus().getExtension(ClientLifeCycleManager.class); if (null != mgr) { mgr.clientDestroyed(new FrontendClientAdapter(getConfiguration())); } - if (cfg.getConduitSelector() instanceof Closeable) { - try { - ((Closeable)cfg.getConduitSelector()).close(); - } catch (IOException e) { - //ignore, we're destroying anyway + if (shutdownConfiguration) { + if (cfg.getConduitSelector() instanceof Closeable) { + try { + ((Closeable)cfg.getConduitSelector()).close(); + } catch (IOException e) { + //ignore, we're destroying anyway + } + } else { + cfg.getConduit().close(); } - } else { - cfg.getConduit().close(); } + + // reset state state.reset(); - if (cfg.isShutdownBusOnClose()) { + + if (shutdownConfiguration && cfg.isShutdownBusOnClose()) { cfg.getBus().shutdown(false); } + state = null; cfg = null; + configurationReference = null; } } @@ -908,6 +950,7 @@ public ClientConfiguration getConfiguration() { protected void setConfiguration(ClientConfiguration config) { cfg = config; + configurationReference = new ConfigurationReference(config); } // Note that some conduit selectors may update Message.ENDPOINT_ADDRESS @@ -1328,4 +1371,26 @@ protected void closeAsyncResponseIfPossible(Response r, Message outMessage, Jaxr protected void handleAsyncFault(Message message) { } } + + /** + * References the configuration (by another client) so it won't shut down till all the + * client instances are closed. + * @param reference configuration reference + */ + protected void setConfigurationReference(ConfigurationReference reference) { + if (reference == null) { + throw new IllegalArgumentException("The configuration reference is not set " + + "(the client was already closed)"); + } + this.configurationReference = reference.acquire(); + this.cfg = configurationReference.getConfiguration(); + } + + /** + * Returns configuration reference + * @return configuration reference + */ + protected @Nullable ConfigurationReference getConfigurationReference() { + return this.configurationReference; + } } diff --git a/rt/rs/client/src/main/java/org/apache/cxf/jaxrs/client/WebClient.java b/rt/rs/client/src/main/java/org/apache/cxf/jaxrs/client/WebClient.java index e0db63b1587..4547a61e570 100644 --- a/rt/rs/client/src/main/java/org/apache/cxf/jaxrs/client/WebClient.java +++ b/rt/rs/client/src/main/java/org/apache/cxf/jaxrs/client/WebClient.java @@ -1232,7 +1232,7 @@ protected void doWriteBody(Message outMessage, static void copyProperties(Client toClient, Client fromClient) { AbstractClient newClient = toAbstractClient(toClient); AbstractClient oldClient = toAbstractClient(fromClient); - newClient.setConfiguration(oldClient.getConfiguration()); + newClient.setConfigurationReference(oldClient.getConfigurationReference()); } private static AbstractClient toAbstractClient(Object client) { diff --git a/rt/rs/client/src/test/java/org/apache/cxf/jaxrs/client/WebClientTest.java b/rt/rs/client/src/test/java/org/apache/cxf/jaxrs/client/WebClientTest.java index b98a68b1c03..a246bf9d360 100644 --- a/rt/rs/client/src/test/java/org/apache/cxf/jaxrs/client/WebClientTest.java +++ b/rt/rs/client/src/test/java/org/apache/cxf/jaxrs/client/WebClientTest.java @@ -28,19 +28,23 @@ import jakarta.ws.rs.core.HttpHeaders; import jakarta.ws.rs.ext.ParamConverter; import jakarta.ws.rs.ext.ParamConverterProvider; +import org.apache.cxf.Bus.BusState; import org.apache.cxf.jaxrs.resources.BookInterface; import org.apache.cxf.jaxrs.resources.BookStore; import org.junit.Test; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.CoreMatchers.nullValue; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.collection.IsIterableContainingInAnyOrder.containsInAnyOrder; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; - public class WebClientTest { @Test @@ -373,6 +377,31 @@ public void testCookieBuilder() { containsInAnyOrder("$Version=1;a=1", "$Version=1;b=2")); } + @Test + public void testWebClientFrom() { + final WebClient wc = WebClient.create("http://foo").language("en_CA"); + wc.getConfiguration().setShutdownBusOnClose(true); + + assertThat(wc.getConfigurationReference(), is(not(nullValue()))); + assertThat(wc.getConfigurationReference().refCount(), equalTo(1L)); + + final WebClient wc1 = WebClient.fromClient(wc); + assertThat(wc.getConfigurationReference(), equalTo(wc1.getConfigurationReference())); + assertThat(wc.getConfigurationReference().refCount(), equalTo(2L)); + wc.close(); + + final ClientConfiguration configuration = wc1.getConfiguration(); + assertThat(configuration, is(not(nullValue()))); + assertThat(configuration.getBus().getState(), equalTo(BusState.RUNNING)); + + assertThat(wc.getConfigurationReference(), is(nullValue())); + assertThat(wc1.getConfigurationReference().refCount(), equalTo(1L)); + wc1.close(); + + assertThat(wc1.getConfigurationReference(), is(nullValue())); + assertThat(configuration.getBus().getState(), equalTo(BusState.SHUTDOWN)); + } + private static final class ParamConverterProviderImpl implements ParamConverterProvider { @SuppressWarnings("unchecked")