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

Added two progress indicators to inform of the upload/download of byt… #328

Merged
merged 17 commits into from
Apr 29, 2020
Merged
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 39 additions & 8 deletions src/main/java/com/android/volley/toolbox/HurlStack.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.io.FilterInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.HttpURLConnection;
import java.net.URL;
import java.util.ArrayList;
Expand Down Expand Up @@ -107,11 +108,12 @@ public HttpResponse executeRequest(Request<?> request, Map<String, String> addit
// Need to keep the connection open until the stream is consumed by the caller. Wrap the
// stream such that close() will disconnect the connection.
keepConnectionOpen = true;
int contentLength = connection.getContentLength ();
return new HttpResponse(
responseCode,
convertHeaders(connection.getHeaderFields()),
connection.getContentLength(),
new UrlConnectionInputStream(connection));
createInputStream(request, connection, responseCode, contentLength));
} finally {
if (!keepConnectionOpen) {
connection.disconnect();
Expand Down Expand Up @@ -153,7 +155,7 @@ private static boolean hasResponseBody(int requestMethod, int responseCode) {
* Wrapper for a {@link HttpURLConnection}'s InputStream which disconnects the connection on
* stream close.
*/
static class UrlConnectionInputStream extends FilterInputStream {
private static class UrlConnectionInputStream extends FilterInputStream {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can just leave this unchanged. I'm not sure if anything is using it but it shouldn't impact this PR. (I was concerned about increasing this classes visibility to protected, but it's fine to leave as is).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can just leave this unchanged. I'm not sure if anything is using it but it shouldn't impact this PR. (I was concerned about increasing this classes visibility to protected, but it's fine to leave as is).

private final HttpURLConnection mConnection;

UrlConnectionInputStream(HttpURLConnection connection) {
Expand All @@ -168,10 +170,25 @@ public void close() throws IOException {
}
}

/**
* Overload this method for manipulate response stream (Use 'write (byte[] b, int off, int len)' in subclasse).
*
* @param request current request.
* @param connection current connection of request.
* @param responseCode response code.
* @param length size of response stream.
* @return an UrlConnectionInputStream object for read response.
*/
protected FilterInputStream createInputStream (Request<?> request, HttpURLConnection connection,
Copy link
Collaborator

Choose a reason for hiding this comment

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

the return type here should be InputStream, not FilterInputStream. Volley doesn't care/depend on what kind of InputStream gets returned here - any InputStream is fine. The fact that we use FilterInputStream here and in your (presumed) subclass isn't a requirement, just an implementation detail.

int responseCode, int length) {

return new UrlConnectionInputStream (connection);
}

/**
* Initializes an {@link InputStream} from the given {@link HttpURLConnection}.
*
* @param connection
* @param connection Request connection
* @return an HttpEntity populated with data from <code>connection</code>.
*/
private static InputStream inputStreamFromConnection(HttpURLConnection connection) {
Expand Down Expand Up @@ -223,7 +240,7 @@ private HttpURLConnection openConnection(URL url, Request<?> request) throws IOE
// NOTE: Any request headers added here (via setRequestProperty or addRequestProperty) should be
// checked against the existing properties in the connection and not overridden if already set.
@SuppressWarnings("deprecation")
/* package */ static void setConnectionParametersForRequest(
/* package */ void setConnectionParametersForRequest(
HttpURLConnection connection, Request<?> request) throws IOException, AuthFailureError {
switch (request.getMethod()) {
case Method.DEPRECATED_GET_OR_POST:
Expand Down Expand Up @@ -270,15 +287,15 @@ private HttpURLConnection openConnection(URL url, Request<?> request) throws IOE
}
}

private static void addBodyIfExists(HttpURLConnection connection, Request<?> request)
private void addBodyIfExists(HttpURLConnection connection, Request<?> request)
throws IOException, AuthFailureError {
byte[] body = request.getBody();
if (body != null) {
addBody(connection, request, body);
}
}

private static void addBody(HttpURLConnection connection, Request<?> request, byte[] body)
private void addBody(HttpURLConnection connection, Request<?> request, byte[] body)
throws IOException {
// Prepare output. There is no need to set Content-Length explicitly,
// since this is handled by HttpURLConnection using the size of the prepared
Expand All @@ -289,8 +306,22 @@ private static void addBody(HttpURLConnection connection, Request<?> request, by
connection.setRequestProperty(
HttpHeaderParser.HEADER_CONTENT_TYPE, request.getBodyContentType());
}
DataOutputStream out = new DataOutputStream(connection.getOutputStream());
OutputStream out = this.createOutputStream ( request, connection, body.length );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be:

DataOutputStream out = new DataOutputStream(createOutputStream(request, connection, body.length));
out.write(body);
out.close();

out.write(body);
out.close();
out.close ();
}

/**
* Overload this method for manipulate request stream (Use 'write (byte[] b)' in subclasse).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two comments here:

  1. I think the wording could be clearer. In general a Javadoc should have a one-sentence summary of what the method needs to do, and then any additional context can come in other paragraphs. For example:

    "Create and return an OutputStream to which the request body will be written.

    May be overridden by subclasses to manipulate or monitor this output stream."

  2. My other comment is what the "Use write(byte[] b) in subclasse(s)" is referencing. What does this mean? In Volley itself, I'm assuming it's fine for us to use any valid method in OutputStream's signature to write data. We currently use write(byte[]) but I don't think we can guarantee always doing this in the future. But that's in Volley itself. In subclasses, why would you ever be calling write()?

    I think what you're saying here is that subclasses can override write(byte[]) to monitor the write. If so, I don't think we should document this, because that might change (maybe we buffer the writes and do it in chunks, for example). All we guarantee is that we'll write the bytes using OutputStream. Your subclass and subclasses in general should monitor any and all methods you care about (e.g. all three write overloads) and support any of them being called.

*
* @param request current request.
* @param connection current connection of request.
* @param length size of stream to write.
* @return an OutputStream object for write request body.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: "@return an OutputStream to which the request body will be written"

*/
protected OutputStream createOutputStream (Request<?> request, HttpURLConnection connection,
int length) throws IOException {

return new DataOutputStream (connection.getOutputStream ());
Copy link
Collaborator

Choose a reason for hiding this comment

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

return connection.getOutputStream()

}
}