Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RESTEASY-3135] Initial change for removing finalizers. #3102

Merged
merged 1 commit into from Jul 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -86,9 +86,4 @@ protected InputPart extractPart(BodyPart bodyPart) throws IOException {

return currPart;
}

protected void finalize() throws Throwable
{
close();
}
}
Expand Up @@ -11,6 +11,7 @@
import org.apache.james.mime4j.stream.Field;
import org.jboss.logging.Logger;
import org.jboss.resteasy.core.ProvidersContextRetainer;
import org.jboss.resteasy.core.ResourceCleaner;
import org.jboss.resteasy.core.ResteasyContext;
import org.jboss.resteasy.spi.HttpRequest;
import org.jboss.resteasy.util.CaseInsensitiveMap;
Expand All @@ -29,6 +30,7 @@
import java.io.SequenceInputStream;
import java.io.UnsupportedEncodingException;
import java.lang.annotation.Annotation;
import java.lang.ref.Cleaner;
import java.lang.reflect.Type;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
Expand All @@ -43,9 +45,21 @@
*/
public class MultipartInputImpl implements MultipartInput, ProvidersContextRetainer
{
private static class CleanupAction implements Runnable {
private volatile Message mimeMessage;

@Override
public void run() {
final Message message = mimeMessage;
if (message != null) {
message.dispose();
}
}
}
protected MediaType contentType;
protected Providers workers;
protected Message mimeMessage;
private final CleanupAction cleanupAction;
private final Cleaner.Cleanable cleanable;
protected List<InputPart> parts = new ArrayList<InputPart>();
protected static final Annotation[] empty = {};
protected MediaType defaultPartContentType = MultipartConstants.TEXT_PLAIN_WITH_CHARSET_US_ASCII_TYPE;
Expand All @@ -56,6 +70,8 @@ public MultipartInputImpl(final MediaType contentType, final Providers workers)
{
this.contentType = contentType;
this.workers = workers;
this.cleanupAction = new CleanupAction();
cleanable = ResourceCleaner.register(this, cleanupAction);
HttpRequest httpRequest = ResteasyContext.getContextData(HttpRequest.class);
if (httpRequest != null)
{
Expand All @@ -80,6 +96,8 @@ public MultipartInputImpl(final MediaType contentType, final Providers workers,
{
this.contentType = contentType;
this.workers = workers;
this.cleanupAction = new CleanupAction();
cleanable = ResourceCleaner.register(this, cleanupAction);
if (defaultPartContentType != null) this.defaultPartContentType = defaultPartContentType;
this.defaultPartCharset = defaultPartCharset;
if (defaultPartCharset != null)
Expand All @@ -97,11 +115,13 @@ public MultipartInputImpl(final Multipart multipart, final Providers workers) th
}
}
this.workers = workers;
this.cleanupAction = new CleanupAction();
cleanable = ResourceCleaner.register(this, cleanupAction);
}

public void parse(InputStream is) throws IOException
{
mimeMessage = Mime4JWorkaround.parseMessage(addHeaderToHeadlessStream(is));
cleanupAction.mimeMessage = Mime4JWorkaround.parseMessage(addHeaderToHeadlessStream(is));
extractParts();
}

Expand All @@ -121,7 +141,7 @@ protected InputStream createHeaderInputStream()

public String getPreamble()
{
return ((Multipart) mimeMessage.getBody()).getPreamble();
return ((Multipart) getMimeMessage().getBody()).getPreamble();
}

public List<InputPart> getParts()
Expand All @@ -131,7 +151,7 @@ public List<InputPart> getParts()

protected void extractParts() throws IOException
{
Multipart multipart = (Multipart) mimeMessage.getBody();
Multipart multipart = (Multipart) getMimeMessage().getBody();
for (Entity entity : multipart.getBodyParts()) {
if (entity instanceof BodyPart)
{
Expand All @@ -145,6 +165,10 @@ protected InputPart extractPart(BodyPart bodyPart) throws IOException
return new PartImpl(bodyPart);
}

protected Message getMimeMessage() {
return cleanupAction.mimeMessage;
}

public class PartImpl implements InputPart
{

Expand Down Expand Up @@ -339,22 +363,7 @@ public static void main(String[] args) throws Exception
@Override
public void close()
{
if (mimeMessage != null)
{
try
{
mimeMessage.dispose();
}
catch (Exception e)
{

}
}
}

protected void finalize() throws Throwable
{
close();
cleanable.clean();
}

protected String getCharset(MediaType mediaType)
Expand Down
Expand Up @@ -39,7 +39,7 @@ public MultipartRelatedInputImpl (final Multipart multipart, final Providers wor
@Override
public void parse(InputStream is) throws IOException {
super.parse(is);
ContentTypeField contentTypeField = (ContentTypeField) mimeMessage
ContentTypeField contentTypeField = (ContentTypeField) getMimeMessage()
.getHeader().getField(FieldName.CONTENT_TYPE);
start = contentTypeField.getParameter("start");
startInfo = contentTypeField.getParameter("start-info");
Expand Down
Expand Up @@ -2,7 +2,6 @@

import org.apache.http.HttpHost;
import org.apache.http.client.HttpClient;
import org.jboss.resteasy.client.jaxrs.i18n.LogMessages;

/**
* An Apache HTTP engine for use with the new Builder Config style.
Expand Down Expand Up @@ -34,12 +33,4 @@ public ApacheHttpClient43Engine(final HttpClient httpClient, final HttpContextPr
super(httpClient, httpContextProvider);
}

public void finalize() throws Throwable
{
if (!closed && allowClosingHttpClient && httpClient != null)
LogMessages.LOGGER.closingForYou(this.getClass());
close();
super.finalize();
}

}
Expand Up @@ -23,6 +23,7 @@
import org.jboss.resteasy.client.jaxrs.internal.ClientInvocation;
import org.jboss.resteasy.client.jaxrs.internal.ClientResponse;
import org.jboss.resteasy.client.jaxrs.internal.FinalizedClientResponse;
import org.jboss.resteasy.core.ResourceCleaner;
import org.jboss.resteasy.spi.config.ConfigurationFactory;
import org.jboss.resteasy.util.CaseInsensitiveMap;

Expand All @@ -39,6 +40,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.lang.management.ManagementFactory;
import java.lang.ref.Cleaner;
import java.nio.file.Path;
import java.security.AccessController;
import java.security.PrivilegedAction;
Expand All @@ -47,12 +49,36 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicBoolean;

/**
* An Apache HTTP engine for use with the new Builder Config style.
*/
public class ManualClosingApacheHttpClient43Engine implements ApacheHttpClientEngine
{
private static class CleanupAction implements Runnable {
private final AtomicBoolean closed;
private final CloseableHttpClient client;

private CleanupAction(final AtomicBoolean closed, final CloseableHttpClient client) {
this.closed = closed;
this.client = client;
}

@Override
public void run() {
if (closed.compareAndSet(false, true)) {
if (client != null) {
LogMessages.LOGGER.closingForYou(this.getClass());
jamezp marked this conversation as resolved.
Show resolved Hide resolved
try {
client.close();
} catch (Exception e) {
LogMessages.LOGGER.debugf(e, "Failed to close client %s", client);
}
}
}
}
}

static final String FILE_UPLOAD_IN_MEMORY_THRESHOLD_PROPERTY =
"org.jboss.resteasy.client.jaxrs.engines.fileUploadInMemoryThreshold";
Expand All @@ -79,7 +105,8 @@ public String run() throws Exception

protected final HttpClient httpClient;

protected boolean closed;
protected final AtomicBoolean closed;
private final Cleaner.Cleanable cleanable;

protected final boolean allowClosingHttpClient;

Expand Down Expand Up @@ -161,6 +188,8 @@ private ManualClosingApacheHttpClient43Engine(final HttpClient httpClient,
}
this.httpContextProvider = httpContextProvider;
this.allowClosingHttpClient = closeHttpClient;
closed = new AtomicBoolean(false);
this.cleanable = createCleanable(this, closeHttpClient, closed, this.httpClient);
this.defaultProxy = defaultProxy;

try
Expand Down Expand Up @@ -681,26 +710,20 @@ private RequestConfig getCurrentConfiguration(final ClientInvocation request, fi

public boolean isClosed()
{
return closed;
return closed.get();
}

public void close()
{
if (closed)
return;
@Override
public void close() {
cleanable.clean();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear about this. cleanable.clean() will set the AtomicBoolean closed to true. Then, if the CleanupAction gets called, closed.compareAndSet(false, true) returns false and client.close() doesn't happen. What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When cleanable.clean() is invoked it's unregistered from the cleaner so it won't be cleaned again. https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ref/Cleaner.Cleanable.html#clean()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was stuck on

() -> closed.set(true)

instead of

ResourceCleaner.register(engine, new CleanupAction(closed, (CloseableHttpClient) client))

for some reason.

It makes sense now.

Looks good to me. What else needs to be done?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you approved it, it just needs to be merged :)


if (allowClosingHttpClient && httpClient != null)
{
try
{
((CloseableHttpClient) httpClient).close();
}
catch (Exception e)
{
throw new RuntimeException(e);
}
private static Cleaner.Cleanable createCleanable(final ManualClosingApacheHttpClient43Engine engine, final boolean allowClose,
final AtomicBoolean closed, final HttpClient client) {
if (allowClose && client instanceof CloseableHttpClient) {
return ResourceCleaner.register(engine, new CleanupAction(closed, (CloseableHttpClient) client));
}
closed = true;
return () -> closed.set(true);
}

private static Path getTempDir() {
Expand Down
Expand Up @@ -5,7 +5,10 @@
/**
* A class that adds a {@link Object#finalize) method to the {@link ClientResponse} as a last ditch backstop to prevent
* leaking resources with ill-behaved clients. Use of finalize could incur a significant performance penalty.
*
* @deprecated {@linkplain #finalize() finalizers} should no longer be used and this type will be removed in the future
*/
@Deprecated
public abstract class FinalizedClientResponse extends ClientResponse {

protected FinalizedClientResponse(final ClientConfiguration configuration,
Expand Down
@@ -0,0 +1,50 @@
/*
* JBoss, Home of Professional Open Source.
*
* Copyright 2022 Red Hat, Inc., and individual contributors
* as indicated by the @author tags.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.jboss.resteasy.core;

import java.lang.ref.Cleaner;

/**
* A utility used to register resources to be cleaned with a {@link Cleaner}.
*
* @author <a href="mailto:jperkins@redhat.com">James R. Perkins</a>
*/
public class ResourceCleaner {

private static final Cleaner CLEANER = Cleaner.create();

private ResourceCleaner() {
}

/**
* Register an object and an action to clean up resources associated with the object once the object instance
* becomes phantom unreachable.
*
* @param instance the instance to monitor
* @param action the action to run for the clean up
*
* @return a cleanable instance
*
* @see Cleaner
*/
public static Cleaner.Cleanable register(final Object instance, final Runnable action) {
return CLEANER.register(instance, action);
}
}
Expand Up @@ -65,7 +65,7 @@ public void testApacheHttpClient4ExecutorNonSharedHttpClientFinalize() throws Th
ResteasyClient client = ((ResteasyClientBuilder)ClientBuilder.newBuilder()).httpEngine(engine).build();
Response response = client.target(generateURL("/test")).request().post(null);
Assert.assertEquals(HttpResponseCodes.SC_NO_CONTENT, response.getStatus());
engine.finalize();
engine.close();
HttpClient httpClient = engine.getHttpClient();
HttpPost post = new HttpPost(generateURL("/test"));
try {
Expand Down Expand Up @@ -114,7 +114,7 @@ public void testApacheHttpClient4ExecutorSharedHttpClientFinalize() throws Throw
ResteasyClient client = ((ResteasyClientBuilder)ClientBuilder.newBuilder()).httpEngine(engine).build();
Response response = client.target(generateURL("/test")).request().post(null);
Assert.assertEquals(HttpResponseCodes.SC_NO_CONTENT, response.getStatus());
engine.finalize();
engine.close();
Assert.assertEquals("Original httpclient and engine httpclient are not the same instance",
httpClient, engine.getHttpClient());
HttpPost post = new HttpPost(generateURL("/test"));
Expand Down