Kong / unirest-java

Unirest in Java: Simplified, lightweight HTTP client library.
http://kong.github.io/unirest-java/
MIT License
2.6k stars 594 forks source link

downloadMonitor return wrong bytesWritten value #469

Closed TheOnlyAndLastOne closed 1 year ago

TheOnlyAndLastOne commented 1 year ago

when I print bytesWritten and totalBytes in my project, I found that bytesWritten is great than totalBytes in the end. by the way, the image was downloaded correctly.

image image
ryber commented 1 year ago

The TotalBytes comes from the Content-Length header returned by the server, so if its wrong, thats kind of on them, I suppose maybe we could start to push it up once the actual bytes is greater than what they said it was going to be? I've also seen the header be greater than the bytes eventually written

TheOnlyAndLastOne commented 1 year ago
image

The first one is download via code, the second via chrome, it seems that totalBytes is right, totalBytes comes from the Content-Length header returned by the server, so where bytesWritten comes from.

ryber commented 1 year ago

The running bytes are summed from a MonitoringInputStream which simply counts the bytes as it proxies the original InputStream.

There are several unit tests in the project that run a real Jetty server and download content with the monitor. In these tests, at the end the Content-Length equals the total consumed.

I don't have access to the file or server you referenced, so I cannot verify with your data. Can you produce an example where this happens that we can duplicate?

TheOnlyAndLastOne commented 1 year ago

This is my local code:

public static void main(String[] args) {
    String catDownloadPath = "/Users/peterzhao/Downloads/test.jpg";
    String cat = "https://cdn.pixabay.com/photo/2021/10/01/18/53/corgi-6673343_960_720.jpg";

    Unirest.get(cat).downloadMonitor((field, fileName, bytesWritten, totalBytes) -> {
        System.out.println(String.format("cat totalBytes: %s, bytesWritten: %s", totalBytes, bytesWritten));
    }).asFile(catDownloadPath);
}
image

I run the code in the main method, there some debug message, the Content-Length equals totalBytes.

public static void main(String[] args) {
    String cityDownloadPath = "/Users/peterzhao/Downloads/city.png";
    String city = "https://cdn.pixabay.com/photo/2023/02/09/16/36/bridge-7779222_1280.jpg";
    Unirest.get(city).downloadMonitor((field, fileName, bytesWritten, totalBytes) -> {
        System.out.println(String.format("city totalBytes: %s, bytesWritten: %s", totalBytes, bytesWritten));
    }).asFile(cityDownloadPath);
}
image

And I tried to download a new one, the same problem, but I found that bytesWritten seems like equal to double totalBytes, is it possible that bytesWritten was counted twice?

ryber commented 1 year ago

weird, I tried both and in both cases I get totalBytes equal to bytes written.

TheOnlyAndLastOne commented 1 year ago

It seems that my previous guess is right, the read bytes were counted twice.

The MonitoringInputStream class override read(byte[] b) and read(byte[] b, int off, int len) method, but read(byte[] b) in super InputStream class called read(byte[] b, int off, int len).

So method read(byte[] b) and read(byte[] b, int off, int len) both called in once file read, and byteCount was added twice.

Please confirm findings above are correct or not thanks.

ryber commented 1 year ago

huh, I think it must depend on exactly which kind of InputStream is being monitored and which methods are being called the override for read(byte[] b) can be removed entirely and it doesn't fail the tests at all. I would like to find a way to create a failing test for this

ryber commented 1 year ago

checkout this https://github.com/Kong/unirest-java/commit/f01154adc25317d881ba7577badcc651958ee731

I'm honestly not entirely sure how the old one worked at all. It should never be calling its own methods. It should be delegating

TheOnlyAndLastOne commented 1 year ago

BytesWritten is correct now except for the last one, I think it may be because the last read() method returns -1?

image
ryber commented 1 year ago

aaaaaaaaah, yea thats why, I was wondering where that missing bit was

TheOnlyAndLastOne commented 1 year ago

The code works as expected now, appreciate