You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/06/09 18:29:25 UTC
[GitHub] [commons-vfs] Aklakan opened a new pull request #186: Fix for VFS-805: HTTP seek always exhausts response
Aklakan opened a new pull request #186:
URL: https://github.com/apache/commons-vfs/pull/186
[Link to VFS-805 on Jira](https://issues.apache.org/jira/browse/VFS-805)
This PR affects the http4 and http5 URI schemes. The http3 scheme is unaffected because it does not use the `MonitoredHttpResponseContentInputStream` abstraction.
With this PR, closing a `MonitoredHttpResponseContentInputStream` no longer closes the underlying stream which may download *all* remaining data. The HttpResponse itself is still closed of course.
The `http5.MonitoredHttpResponseContentInputStream` variant in addition required replacing the http entity with a dummy one during close in order to prevent downloading all remaining data: In contrast to hc4, closing the http response with hc5 now also closes the entity. Interestingly, replacing the entity is possible and does not close an existing one. Not sure about how stable this behavior is, but I couldn't find any reasonable alternative. For example, `CloseableHttpResponse` has a package private constructor, so it's not possible to create a custom http response interceptor that creates an instance of a custom subclass of `CloseableHttpResponse` that doesn't close the entity.
Also, I am not sure about how to best provide a test case:
* While I see that the unit tests spawn an http server, I am not sure how much effort it would be in order for that server to *track statistics* about how much content was retrieved.
* HttpClient 3/4/5 all return that ContentLengthInputStream if the HTTP header includes a `Content-Length` entry - this part is pretty much hard coded in all version of hc. So there doesn't seem to be a simple way to inject a custom InputStream implementation that tracks how much data is being read.
Locally, I tested this change with the benchmark described in the Jira issue and let it perform 100k iterations of seeking over a 2gb file in order to potentially reveal connection leaks. For me its working.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-vfs] garydgregory commented on a change in pull request #186: VFS-805 - HTTP seek always exhausts response
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #186:
URL: https://github.com/apache/commons-vfs/pull/186#discussion_r670587749
##########
File path: commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/http5/MonitoredHttpResponseContentInputStream.java
##########
@@ -38,8 +41,25 @@ public MonitoredHttpResponseContentInputStream(final ClassicHttpResponse httpRes
this.httpResponse = httpResponse;
}
+ /**
+ * Prevent closing the stream itself if the httpResponse is closeable.
+ * Closing the stream may consume all remaining data no matter how large (VFS-805).
+ */
+ @Override
+ protected void closeSuper() throws IOException {
+ // Suppressed close() invocation on the underlying input stream
+ }
+
@Override
protected void onClose() throws IOException {
+ // Replace the response's entity with a dummy entity in order to prevent
+ // exhausting all data (VFS-805)
+ // Note: HC 5.1 introduces a dedicated NullEntity class
Review comment:
@Aklakan
Our HC 5 code base is now on HC 5.1
##########
File path: commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/http5/MonitoredHttpResponseContentInputStream.java
##########
@@ -38,8 +41,25 @@ public MonitoredHttpResponseContentInputStream(final ClassicHttpResponse httpRes
this.httpResponse = httpResponse;
}
+ /**
+ * Prevent closing the stream itself if the httpResponse is closeable.
+ * Closing the stream may consume all remaining data no matter how large (VFS-805).
+ */
+ @Override
+ protected void closeSuper() throws IOException {
+ // Suppressed close() invocation on the underlying input stream
+ }
+
@Override
protected void onClose() throws IOException {
+ // Replace the response's entity with a dummy entity in order to prevent
+ // exhausting all data (VFS-805)
+ // Note: HC 5.1 introduces a dedicated NullEntity class
+ HttpEntity nullEntity = new ByteArrayEntity(new byte[0], ContentType.APPLICATION_OCTET_STREAM);
+
Review comment:
No need for extra blank lines.
##########
File path: commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/http5/MonitoredHttpResponseContentInputStream.java
##########
@@ -38,8 +41,25 @@ public MonitoredHttpResponseContentInputStream(final ClassicHttpResponse httpRes
this.httpResponse = httpResponse;
}
+ /**
+ * Prevent closing the stream itself if the httpResponse is closeable.
+ * Closing the stream may consume all remaining data no matter how large (VFS-805).
+ */
+ @Override
+ protected void closeSuper() throws IOException {
+ // Suppressed close() invocation on the underlying input stream
+ }
+
@Override
protected void onClose() throws IOException {
+ // Replace the response's entity with a dummy entity in order to prevent
+ // exhausting all data (VFS-805)
+ // Note: HC 5.1 introduces a dedicated NullEntity class
+ HttpEntity nullEntity = new ByteArrayEntity(new byte[0], ContentType.APPLICATION_OCTET_STREAM);
+
+ httpResponse.setEntity(nullEntity);
Review comment:
```
httpResponse.setEntity(NullEntity.INSTANCE);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-vfs] garydgregory commented on pull request #186: VFS-805 - HTTP seek always exhausts response
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #186:
URL: https://github.com/apache/commons-vfs/pull/186#issuecomment-880973762
@Aklakan
In git master now, with adjustments.
TY for you PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-vfs] garydgregory closed pull request #186: VFS-805 - HTTP seek always exhausts response
Posted by GitBox <gi...@apache.org>.
garydgregory closed pull request #186:
URL: https://github.com/apache/commons-vfs/pull/186
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-vfs] garydgregory commented on pull request #186: VFS-805 - HTTP seek always exhausts response
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #186:
URL: https://github.com/apache/commons-vfs/pull/186#issuecomment-877308593
It seems the best way to test this is with mocking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org