Open
Description
#760 and #307 (and possible others -- I'm afraid I can't figure out how to properly search for issues in this system) are special cases of this ticket.
It looks like the server doesn't always close Representation instances when it's done with them. For example, a HEAD request is implemented as a GET request where the Representation entity is null'd out from the response prior to sending the response. But the Representation isn't released when null'd. That leads to a resource leak, such as those of issues #760 and #307, which can be quite bad.
My first thought was that Message.setEntity(Representation) should always release any preexisting entity before setting the new one. But I don't need to tell you what a bad idea that is, because, for example, it would break Decoder and Encoder which wrap the old Representation.
So instead I've added clearEntity() and resetEntity(Representation) methods to Message, and have made an attempt to audit the code to find which setEntity calls should actually call clearEntity or resetEntity instead. As I can't figure out how to attach a patch that fixes the issue I was seeing with HEAD requests, I'll quote it below (sorry). But I wouldn't trust my audit. For example, I wasn't sure about ClientAdapter.updateResponse(Response, Status, ClientCall) (around line 203). Also, I only looked at org.restlet, and not org.restlet.ext.*
diff -u -r ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/Message.java org.restlet/org/restlet/Message.java
--- ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/Message.java 2013-09-07 16:40:54.000000000 -0500
+++ org.restlet/org/restlet/Message.java 2013-11-13 10:09:44.000000000 -0600
@@ -375,10 +375,35 @@
*
* @param entity
* The entity representation.
+ * @see #resetEntity(Representation)
+ * @see #clearEntity()
*/
public void setEntity(Representation entity) {
this.entity = entity;
}
+
+ /**
+ * Clears the entity representation.
+ * This method is equivalent to {@link #reset(Representation) reset(null)}.
+ */
+ public void clearEntity() {
+ resetEntity(null);
+ }
+
+ /**
+ * Resets the entity representation.
+ * This method is similar to {@link #setEntity(Representation)
+ * setEntity(entity)} but also {@linkplain Representation#release() releases}
+ * any preexisting entity.
+ *
+ * @param entity
+ * The entity representation.
+ */
+ public void resetEntity(Representation entity) {
+ Representation oldEntity = getEntity(); // works in subclasses too
+ if (null != oldEntity) oldEntity.release();
+ setEntity(entity);
+ }
/**
* Sets a textual entity.
diff -u -r ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/adapter/ClientAdapter.java org.restlet/org/restlet/engine/adapter/ClientAdapter.java
--- ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/adapter/ClientAdapter.java 2013-09-07 16:40:54.000000000 -0500
+++ org.restlet/org/restlet/engine/adapter/ClientAdapter.java 2013-11-06 16:40:53.000000000 -0600
@@ -199,7 +199,7 @@
// Read the response headers
readResponseHeaders(httpCall, response);
- // Set the entity
+ // Set the entity. TODO: does the old one need to be released?
response.setEntity(httpCall.getResponseEntity(response));
// Release the representation's content for some obvious cases
diff -u -r ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/adapter/ServerAdapter.java org.restlet/org/restlet/engine/adapter/ServerAdapter.java
--- ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/adapter/ServerAdapter.java 2013-09-07 16:40:54.000000000 -0500
+++ org.restlet/org/restlet/engine/adapter/ServerAdapter.java 2013-11-06 16:49:53.000000000 -0600
@@ -123,7 +123,7 @@
if ((response.getRequest().getMethod() != null)
&& response.getRequest().getMethod().equals(Method.HEAD)) {
addEntityHeaders(response);
- response.setEntity(null);
+ response.clearEntity();
} else if (Method.GET.equals(response.getRequest().getMethod())
&& Status.SUCCESS_OK.equals(response.getStatus())
&& (!response.isEntityAvailable())) {
@@ -142,7 +142,7 @@
.fine("Responses with a 204 (No content) status generally don't have an entity. Only adding entity headers for resource \""
+ response.getRequest().getResourceRef()
+ "\".");
- response.setEntity(null);
+ response.clearEntity();
}
} else if (response.getStatus()
.equals(Status.SUCCESS_RESET_CONTENT)) {
@@ -152,7 +152,7 @@
"Responses with a 205 (Reset content) status can't have an entity. Ignoring the entity for resource \""
+ response.getRequest()
.getResourceRef() + "\".");
- response.setEntity(null);
+ response.clearEntity();
}
} else if (response.getStatus().equals(
Status.REDIRECTION_NOT_MODIFIED)) {
@@ -160,7 +160,7 @@
HeaderUtils.addNotModifiedEntityHeaders(response
.getEntity(), response.getHttpCall()
.getResponseHeaders());
- response.setEntity(null);
+ response.clearEntity();
}
} else if (response.getStatus().isInformational()) {
if (response.isEntityAvailable()) {
@@ -169,7 +169,7 @@
"Responses with an informational (1xx) status can't have an entity. Ignoring the entity for resource \""
+ response.getRequest()
.getResourceRef() + "\".");
- response.setEntity(null);
+ response.clearEntity();
}
} else {
addEntityHeaders(response);
@@ -185,7 +185,7 @@
+ "\".");
}
- response.setEntity(null);
+ response.clearEntity();
}
}
@@ -208,7 +208,7 @@
Status.SERVER_ERROR_INTERNAL.getCode());
response.getHttpCall().setReasonPhrase(
"An exception occured writing the response entity");
- response.setEntity(null);
+ response.clearEntity();
try {
response.getHttpCall().sendResponse(response);
diff -u -r ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/application/RangeFilter.java org.restlet/org/restlet/engine/application/RangeFilter.java
--- ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/application/RangeFilter.java 2013-09-07 16:40:54.000000000 -0500
+++ org.restlet/org/restlet/engine/application/RangeFilter.java 2013-11-06 16:51:03.000000000 -0600
@@ -98,7 +98,7 @@
getLogger()
.warning(
"Unable to serve this range since at least the end index of the range cannot be computed.");
- response.setEntity(null);
+ response.clearEntity();
} else if (!requestedRange.equals(response
.getEntity().getRange())) {
if (rangedEntity) {
@@ -123,7 +123,7 @@
getLogger()
.warning(
"Multiple ranges are not supported at this time.");
- response.setEntity(null);
+ response.clearEntity();
}
}
} else {
diff -u -r ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/application/StatusFilter.java org.restlet/org/restlet/engine/application/StatusFilter.java
--- ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/application/StatusFilter.java 2013-09-07 16:40:54.000000000 -0500
+++ org.restlet/org/restlet/engine/application/StatusFilter.java 2013-11-06 16:52:27.000000000 -0600
@@ -132,7 +132,7 @@
// Do we need to get a representation for the current status?
if (response.getStatus().isError()
&& ((response.getEntity() == null) || isOverwriting())) {
- response.setEntity(getRepresentation(response.getStatus(), request,
+ response.resetEntity(getRepresentation(response.getStatus(), request,
response));
}
}
diff -u -r ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/connector/ClientInboundWay.java org.restlet/org/restlet/engine/connector/ClientInboundWay.java
--- ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/connector/ClientInboundWay.java 2013-09-07 16:40:54.000000000 -0500
+++ org.restlet/org/restlet/engine/connector/ClientInboundWay.java 2013-11-06 16:53:18.000000000 -0600
@@ -134,7 +134,8 @@
super.onHeadersCompleted();
// Update the response
- getMessage().setEntity(createEntity(getHeaders()));
+ // TODO: make sure resetEntity is correct (or moot)
+ getMessage().resetEntity(createEntity(getHeaders()));
try {
copyResponseTransportHeaders(getHeaders(), getMessage());
diff -u -r ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/connector/ServerInboundWay.java org.restlet/org/restlet/engine/connector/ServerInboundWay.java
--- ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/connector/ServerInboundWay.java 2013-09-07 16:40:54.000000000 -0500
+++ org.restlet/org/restlet/engine/connector/ServerInboundWay.java 2013-11-06 16:54:38.000000000 -0600
@@ -111,7 +111,8 @@
// Check if an entity is available
Representation entity = createEntity(getHeaders());
- getMessage().getRequest().setEntity(entity);
+ // TODO: make sure resetEntity is correct (or moot)
+ getMessage().getRequest().resetEntity(entity);
// Update the response
getMessage().getServerInfo().setAddress(
diff -u -r ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/connector/ServerOutboundWay.java org.restlet/org/restlet/engine/connector/ServerOutboundWay.java
--- ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/connector/ServerOutboundWay.java 2013-09-07 16:40:54.000000000 -0500
+++ org.restlet/org/restlet/engine/connector/ServerOutboundWay.java 2013-11-06 16:55:35.000000000 -0600
@@ -77,19 +77,19 @@
.warning(
"Responses with a 205 (Reset content) status can't have an entity. Ignoring the entity for resource \""
+ request.getResourceRef() + "\".");
- response.setEntity(null);
+ response.clearEntity();
} else if (Status.REDIRECTION_NOT_MODIFIED.equals(response.getStatus())
&& (request.getEntity() != null)) {
HeaderUtils.addNotModifiedEntityHeaders(response.getEntity(),
headers);
- response.setEntity(null);
+ response.clearEntity();
} else if (response.getStatus().isInformational()
&& response.isEntityAvailable()) {
getLogger()
.warning(
"Responses with an informational (1xx) status can't have an entity. Ignoring the entity for resource \""
+ request.getResourceRef() + "\".");
- response.setEntity(null);
+ response.clearEntity();
}
@@ -109,7 +109,7 @@
.getResourceRef() + "\".");
}
- response.setEntity(null);
+ response.clearEntity();
}
if (Method.GET.equals(request.getMethod())
@@ -125,11 +125,11 @@
getLogger()
.fine("Responses with a 204 (No content) status generally don't have an entity available. Only adding entity headers for resource \""
+ request.getResourceRef() + "\".");
- response.setEntity(null);
+ response.clearEntity();
}
if (Method.HEAD.equals(request.getMethod())) {
- response.setEntity(null);
+ response.clearEntity();
}
}
diff -u -r ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/local/ClapClientHelper.java org.restlet/org/restlet/engine/local/ClapClientHelper.java
--- ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/local/ClapClientHelper.java 2013-09-07 16:40:54.000000000 -0500
+++ org.restlet/org/restlet/engine/local/ClapClientHelper.java 2013-11-06 16:56:55.000000000 -0600
@@ -145,7 +145,7 @@
getMetadataService());
// Update the response
- response.setEntity(output);
+ response.resetEntity(output);
response.setStatus(Status.SUCCESS_OK);
} catch (IOException ioe) {
getLogger().log(Level.WARNING,
diff -u -r ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/local/EntityClientHelper.java org.restlet/org/restlet/engine/local/EntityClientHelper.java
--- ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/local/EntityClientHelper.java 2013-09-07 16:40:54.000000000 -0500
+++ org.restlet/org/restlet/engine/local/EntityClientHelper.java 2013-11-06 16:59:37.000000000 -0600
@@ -297,7 +297,7 @@
response.setStatus(Status.CLIENT_ERROR_NOT_FOUND);
} else {
output.setLocationRef(request.getResourceRef());
- response.setEntity(output);
+ response.resetEntity(output);
response.setStatus(Status.SUCCESS_OK);
}
}
diff -u -r ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/local/ZipClientHelper.java org.restlet/org/restlet/engine/local/ZipClientHelper.java
--- ../../../restlet/restlet-jse-2.1.4/src/org.restlet/org/restlet/engine/local/ZipClientHelper.java 2013-09-07 16:40:54.000000000 -0500
+++ org.restlet/org/restlet/engine/local/ZipClientHelper.java 2013-11-06 17:00:02.000000000 -0600
@@ -192,7 +192,7 @@
}
response.setStatus(Status.SUCCESS_OK);
- response.setEntity(output);
+ response.resetEntity(output);
}
}
}