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

ndagnas · 2020-04-20T18:02:57Z

Hi,

That branch provides two listeners in order to follow up on the progress of sending and receiving data.

Thank you for your time,

Best regards,

Nicolas Dagnas

…es during the request execution

googlebot · 2020-04-20T18:03:11Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

ndagnas · 2020-04-20T18:05:05Z

@googlebot I signed it!

googlebot · 2020-04-20T18:05:08Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

jpd236 · 2020-04-21T00:27:25Z

This is an interesting idea and a request we've gotten before, but I'm not sure that in practice it works as neatly as specified here, and I'm not sure it really fits into Volley's broader goals or encourages good practices.

My two high-level concerns are:

The PR makes the assumption that copying the bytes to the relevant stream is equivalent to uploading/downloading progress. In reality, it seems like there are likely layers of buffering between those streams and when those bytes actually get uploaded/downloaded, and so any progress indicator interface that we provide would likely be inaccurate (by indicating the upload was complete before all the data was actually fully transferred, or by lagging behind the actual progress of the download), which seems like it would diminish the value of such an API.

I think it's also the case that the behavior could vary depending on the HTTP stack being used. You've implemented this for HurlStack, but Apache HTTP (though deprecated) is still an option, and there are other HTTP stacks out there as well. Do they expose this type of information? It's okay to have it be an optional feature, but we'd probably need to document it as such and provide a clearer failure mode if it's not fully supported.
Volley is explicitly discouraged for "large" downloads/uploads because it holds all the requests/responses in memory and doesn't handle pausing/resuming in-flight requests. Upload/download progress APIs would run counter to that - if your upload/download is so big that you're worried about how it is progressing, it's very possible that Volley is not the right fit for that operation, and you'd be better off with something that doesn't keep the full request/response in memory and which supports resuming incomplete downloads.

I'd be open to this if there was more evidence that 1) makes sense in light of buffering, or if there was clearly value here despite the potential inaccuracy, and that there were use cases for these APIs even for requests that could reasonably be considered small.

ndagnas · 2020-04-21T07:31:16Z

My objective is to improve the UX by allowing development to indicate progress in the event of a bad connection.

I currently use with files of less than 1Mo (Medical inventory) and it happens that the server is limited.

I provide in this way a differentiated indication between:

Negotiation [volley] (indefinite progress)
Download [volley listener] (Progress: ex 0 -> 80%)
Processing [Me] (Progress: 80% -> 100%)

The user has less of the impression of waiting.

I specify that I do not use it for large files.

I do not want to have exact information but just to be able to improve the UX.

jpd236 · 2020-04-22T00:55:26Z

Thanks for the context. It seems reasonable enough, but at the same time with all the caveats about how realistic these percentages are, I'm hesitant to expose them directly in Volley's API.

I think I'd feel more comfortable keeping the scope of this smaller by providing the hooks you need to build such a listener yourself without having to support the full listener interface or try to describe the caveats such an interface might have. One way to accomplish that would be to expose the request OutputStream and response InputStream from HurlStack so that you could wrap them in your own filter streams which could monitor transfer progress and report it to your application however you wish.

That is, if HurlStack had a protected method to get the InputStream for a connection whose default implementation was "new UrlConnectionInputStream(connection)", you would be able to override it and wrap the superclass implementation's InputStream in your own subclass of FilterInputStream which could override calls to read() and implement your callback that way.

Is that a reasonable option for you? Then all we have to change in Volley itself is adding a couple protected methods to HurlStack for creating the input/output streams for a connection. This keeps the scope of the change smaller and avoids more common code like BasicNetwork, and doesn't expose an API that would be harder to support across all HTTP stacks and which might be inaccurate (which could be fine for you, but not others). And it feels more reasonable to say that HurlStack will always require pulling out an OutputStream to write the request body to and pulling out an InputStream to read the response body from, so allowing subclasses to tweak these implementations feels like a reasonably safe long-term API.

…the request execution in superclass of HurlStack

ndagnas · 2020-04-22T12:44:19Z

Thank you for your detailed reply which clarifies the situation.

That's a perfectly reasonable option for me.

I've therefore withdrawn all my modifications concerning objects other than HurlStack, and I added the two protected methods as you suggested, as well as the modifications relating thereto.

The arguments of these methods are "Request", "Connection", "Length" and "ResponseCode" (Output) objects that might be useful as they are to me.

I hope the result suits you.

…the request execution in superclass of HurlStack

jpd236

Sorry for the delay. I think I support this approach at a high-level but I have some comments to resolve first.

jpd236 · 2020-04-28T19:18:03Z

src/main/java/com/android/volley/toolbox/HurlStack.java

@@ -153,10 +154,10 @@ 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 {
+    protected static class UrlConnectionInputStream extends FilterInputStream {


I would rather not expose this implementation detail as part of Volley's API. I don't think you should need it - instead of extending it, you can build your own FilterInputStream subclass which filters the result of super.createInputStream() using composition rather than inheritance.

jpd236 · 2020-04-28T19:19:29Z

src/main/java/com/android/volley/toolbox/HurlStack.java

@@ -223,7 +239,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 */ static void setConnectionParametersForRequest(HurlStack stack,


Rather than passing HurlStack through static methods, we should probably just make those methods non-static, especially if they're private/package-private helpers.

jpd236 · 2020-04-28T19:20:30Z

src/main/java/com/android/volley/toolbox/HurlStack.java

+     * @param length size of stream to write.
+     * @return an DataOutputStream object for write request body.
+     */
+    protected DataOutputStream createOutputStream (Request<?> request, HttpURLConnection connection, 


We shouldn't require the subclass to create DataOutputStream specifically, as that's an implementation detail. We should just have this method signature require returning an OutputStream, and default to returning connection.getOutputStream(). addBody should still wrap it in a DataOutputStream as before.

…the request execution in superclass of HurlStack

ndagnas · 2020-04-28T19:54:12Z

So much the better if it suits you. I was hesitant to make certain changes as you envisioned.

I was not sure I could afford some changes that could have had consequences.

Our visions being in agreement, I made the modifications that you asked for.

jpd236 · 2020-04-28T20:30:06Z

src/main/java/com/android/volley/toolbox/HurlStack.java

@@ -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 {


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).

jpd236 · 2020-04-28T20:31:00Z

src/main/java/com/android/volley/toolbox/HurlStack.java

+     * @param length size of response stream.
+     * @return an UrlConnectionInputStream object for read response.
+     */
+    protected FilterInputStream createInputStream (Request<?> request, HttpURLConnection connection,


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.

jpd236 · 2020-04-28T20:31:03Z

src/main/java/com/android/volley/toolbox/HurlStack.java

@@ -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 {


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).

jpd236 · 2020-04-28T20:31:58Z

src/main/java/com/android/volley/toolbox/HurlStack.java

@@ -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 );


This should be:

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

jpd236 · 2020-04-28T20:32:11Z

src/main/java/com/android/volley/toolbox/HurlStack.java

+    protected OutputStream createOutputStream (Request<?> request, HttpURLConnection connection,
+		int length) throws IOException {
+
+        return new DataOutputStream (connection.getOutputStream ());


return connection.getOutputStream()

jpd236 · 2020-04-28T20:45:14Z

src/main/java/com/android/volley/toolbox/HurlStack.java

+     * @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.


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

jpd236 · 2020-04-28T20:45:14Z

src/main/java/com/android/volley/toolbox/HurlStack.java

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


Two comments here:

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."

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.

jpd236 · 2020-04-28T20:46:53Z

BTW, there was an issue where our continuous build results weren't being posted to PRs. I've fixed that and rerun the build so you should see the failure now - please be sure to address that.

ndagnas · 2020-04-28T21:54:06Z

If I am not mistaken, I just have to modify the HurlStackText file by replacing:

'HurlStack.setConnectionParametersForRequest...'
with:
'(new HurlStack ()). SetConnectionParametersForRequest...'

Can I do it?

I take care of the last details.

jpd236 · 2020-04-28T23:14:52Z

You should be able to use mHurlStack.setConnectionParametersForRequest.

…the request execution in superclass of HurlStack

ndagnas · 2020-04-29T12:39:07Z

Phew, finally ...

It was not without difficulty to pass 'GoogleJavaFormat', it is very picky.

But I did it. Is it all good suddenly? ^^

- Remove redundant arguments to createInputStream which can be inferred from the provided HttpURLConnection. - Clean up Javadoc. - Add unit tests.

jpd236 · 2020-04-29T20:27:50Z

Sorry about the formatting hassle. For future reference, "./gradlew googleJavaFormat" does the formatting automatically for you.

I think the length and response code arguments to createInputStream were redundant, because we're already passing in the connection object. If the subclass wants these values, it can just call connection.getResponseCode()/getResponseLength() directly.

I removed those arguments, made some small edits to the Javadoc, and added unit tests. Merging.

ndagnas · 2020-04-30T17:04:04Z

Thank you for the correction. I focused so much on the overloads that I had missed out.

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

d7dc44e

…es during the request execution

Added two protected method for check upload/download of bytes during …

26d9d01

…the request execution in superclass of HurlStack

Added two protected method for check upload/download of bytes during …

3b028d0

…the request execution in superclass of HurlStack

jpd236 reviewed Apr 28, 2020

View reviewed changes

Added two protected method for check upload/download of bytes during …

920e7e4

…the request execution in superclass of HurlStack

jpd236 reviewed Apr 28, 2020

View reviewed changes

ndagnas added 12 commits April 29, 2020 08:38

Added two protected method for check upload/download of bytes during …

9fb5dd6

…the request execution in superclass of HurlStack

Added two protected method for check upload/download of bytes during …

ac0b4d7

…the request execution in superclass of HurlStack

Added two protected method for check upload/download of bytes during …

89ed1ee

…the request execution in superclass of HurlStack

Added two protected method for check upload/download of bytes during …

3dafd56

…the request execution in superclass of HurlStack

Added two protected method for check upload/download of bytes during …

f504669

…the request execution in superclass of HurlStack

Added two protected method for check upload/download of bytes during …

ff18fe1

…the request execution in superclass of HurlStack

Added two protected method for check upload/download of bytes during …

6b1568c

…the request execution in superclass of HurlStack

Fixing Google Java Format

cea3b7f

Fixing Google Java Format

d5cad0f

Fixing Google Java Format

0f5b553

Fixing Google Java Format

97df8d6

Fixing Google Java Format

c8a7083

Polish new HurlStack methods.

d31dd6f

- Remove redundant arguments to createInputStream which can be inferred from the provided HttpURLConnection. - Clean up Javadoc. - Add unit tests.

jpd236 merged commit 455a161 into google:master Apr 29, 2020

ndagnas deleted the add-progress-indicator branch May 12, 2020 06:51

ndagnas restored the add-progress-indicator branch May 12, 2020 06:51

ndagnas deleted the add-progress-indicator branch May 12, 2020 06:52

jpd236 mentioned this pull request Feb 15, 2021

how to check received bytes or data continuously until data received fully in volley #396

Closed

This was referenced Mar 9, 2021

Bump volley from 1.1.1 to 1.2.0 in /app grape-networking/grape-networking-java#63

Merged

Bump volley from 1.1.1 to 1.2.0 darkmatter18/Face-Detector#29

Closed

Bump volley from 1.1.1 to 1.2.0 AdamLBS/LineFlag#83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ndagnas commented Apr 20, 2020

googlebot commented Apr 20, 2020

ndagnas commented Apr 20, 2020

googlebot commented Apr 20, 2020

jpd236 commented Apr 21, 2020 •

edited

Loading

ndagnas commented Apr 21, 2020

jpd236 commented Apr 22, 2020

ndagnas commented Apr 22, 2020

jpd236 left a comment

jpd236 Apr 28, 2020

jpd236 Apr 28, 2020

jpd236 Apr 28, 2020

ndagnas commented Apr 28, 2020

jpd236 Apr 28, 2020

jpd236 Apr 28, 2020

jpd236 Apr 28, 2020

jpd236 Apr 28, 2020

jpd236 Apr 28, 2020

jpd236 Apr 28, 2020

jpd236 Apr 28, 2020

jpd236 commented Apr 28, 2020

ndagnas commented Apr 28, 2020

jpd236 commented Apr 28, 2020

ndagnas commented Apr 29, 2020

jpd236 commented Apr 29, 2020

ndagnas commented Apr 30, 2020

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

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

Conversation

ndagnas commented Apr 20, 2020

googlebot commented Apr 20, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

ndagnas commented Apr 20, 2020

googlebot commented Apr 20, 2020

jpd236 commented Apr 21, 2020 • edited Loading

ndagnas commented Apr 21, 2020

jpd236 commented Apr 22, 2020

ndagnas commented Apr 22, 2020

jpd236 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndagnas commented Apr 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpd236 commented Apr 28, 2020

ndagnas commented Apr 28, 2020

jpd236 commented Apr 28, 2020

ndagnas commented Apr 29, 2020

jpd236 commented Apr 29, 2020

ndagnas commented Apr 30, 2020

jpd236 commented Apr 21, 2020 •

edited

Loading