Files are corrupt, when dowloading files via HTTP using Pump

[Joan R.]

Hey there!

i am using vertx 3.0.0 and and i'm trying to download a file via http and save it to disk. Therefore i use a Pump.

The file which is created has exactly the same size (with exactly i mean really exactly:)) and no error occurs.

But unfortunately the downloaded file is corrupt. The file is a videofile but i cant play them with vlc.

The unix programm "diff" also says that the files are not equal.

If i view the original file and the downloaded file with a hextool, i can see that some parts are equal and others not.

I dont know what i'm doing wrong, i implemented the same in vertx 2.X and it works fine.

This is the code which creates the client:

HttpClientOptions options = new HttpClientOptions()
        .setDefaultHost(resolvedUrl.getHost())
        .setDefaultPort(resolvedUrl.getPort());
HttpClient client = vertx.createHttpClient(options);

HttpClientRequest httpClientRequest = client.get(resolvedUrl.getFile(), getHttpClientResponseHandler(asyncFile, filePath, message));
httpClientRequest.exceptionHandler(getExceptionHandler(url, message, asyncFile));
httpClientRequest.end();

Here is my code which handles the httpResponse:

private Handler<HttpClientResponse> getHttpClientResponseHandler(final AsyncFile asyncFile, final String filePath, Message<String> message) {
    return httpClientResponse -> {
        int statusCode = httpClientResponse.statusCode();
        httpClientResponse.pause();

        if (statusCode != 200) {
            String errorMessage = String.format("Could not download file. Status code was not 200, got: %s", statusCode);
            logger.error(errorMessage);
            message.fail(MESSAGE_RESPONSE_CODES.DOWNLOAD_FAILED_NOT_200.getCode(), errorMessage);
            asyncFile.close();
            return;
        }

        httpClientResponse.endHandler(httpEndHandler -> {
            asyncFile.flush(flushResp -> {
                asyncFile.close(event -> {
                    if (event.succeeded()) {
                        logger.info(String.format("Closed file: %s", filePath));
                        message.reply(filePath);
                    } else {
                        logger.info(String.format("Could not close file: %s", filePath));

                    }
                });
            });

        });

        Pump pump = Pump.factory.pump(httpClientResponse, asyncFile);
        httpClientResponse.resume();
        pump.start();
    };

} 

Greetings, 

Joan

[Joan R.]

Update:

I figured out, that the problem is not the receiving side. 

If i try the code above, with another url (for example, this: http://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov) it works perfectly. 

The files is served by another vertx application, which is written in vertx 2.1.5.

The strange thing is, that i can download form this other vertx application with curl, without any problem.

 

[Joan R.]

Update2:

I tried downloading with the above code via nginx and its corrupt again.

it makes no sense....

[Deven Phillips]

Have a look at the examples located here: https://github.com/vert-x3/vertx-examples/tree/2b6f74fd7af731f15834c8cbc7da532437d9c685/core-examples/src/main/java/io/vertx/example/core/http/upload

[Joan R.]

Here my updated current code, which still fails: http://pastebin.com/BZ6mBN8M

[Nat]

Look like you forgot to close your file. Does anything change if you close the file properly?

[Deven Phillips]

I thought that Pump would automatically close the associated streams when the input stream closes?

Deven

--
You received this message because you are subscribed to a topic in the Google Groups "vert.x" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/vertx/N_wSoQlvMMs/unsubscribe.
To unsubscribe from this group and all its topics, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.
To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/004f19bf-74f0-4891-867e-e9e8d37a5231%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Joan R.]

i talked with  deven philipps and he said that too. i changed it, as you can see on pastebit. but is also not works. the file is still corrupt.

any other ideas ? 

[Joan R.]

Update: if we download the file without the pump (using the datahandler and write manually into the file, we now the impact on the ram) its working. so there is something wrong with the pump...

[Nat]

I didn't see your file get updated on pastebin.

[Joan R.]

i didnt uploaded the code which does not use a pump, because it was only a way to validate, that the error is on the receiving side. 
Or did i misunderstood you ?

its alomoast this line of code we use: 

+    private Handler<Buffer> getDataHandler(AsyncFile asyncFile) {

+        return buffer -> {

+            asyncFile.write(buffer);

+        };

+    }

but we cant use this code in production because it can consume too much ram...

i am still looking for a solution which uses the pump to avoid high ram consumption.

[Joan R.]

is there nobody who downloads a file via http using a pump with vertx 3 ? :)

[kim young ill]

if the sizes are the same, it could be that the bytes are written in wrong order (threading-issue)?

On Wed, Aug 5, 2015 at 7:48 AM, Joan R. <joan...@googlemail.com> wrote:

is there nobody who downloads a file via http using a pump with vertx 3 ? :)

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.

Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/98d0ce17-0ee5-4d0d-af20-5a8a4af84313%40googlegroups.com.

[Nat]

Do you have a small/self contained test that exhibits the behavior?

[Joan R.]

Yes the size is exactly the same.

I try to upload a self containig example today.

For me it looks also like a threading issue.

[Joan R.]

Hey there, 

 i have uploaded a test app. you need only adjust the url and path. Keep in mind, that you need a local nginx and a local url.

As i said before, if you download from the internet, it works.

https://github.com/athlordJojo/vertx-pump-issue

greets

[Nat]

I still don't see you close the file when it finishes.

you should make it into something like

     httpClientResponse.endHandler(httpEndHandler -> {

                asyncFile.close(x->{

message.reply(filePath);

});

            });

[Joan R.]

Deven Philips told that the pump closes the streams. I also tried it with closing the file, it doesnt fixed the Error.

[Alexander Lehmann]

Your example works for me when using nginx running on localhost with the big buck bunny file.

I am using Windows though, which may cause a different behaviour though I don't see how. I can try to set up a Linux VM to check the same operation, but that may be take some time.

If you are in fact receiving a file with the same size, it would be interesting to check how the file is wrong (e.g. some blockwise error is introduced, some bytes are duplicated or whatever), this should be possible to check by doing a byte by byte comparison of the file.

[Joan R.]

It happens on osx and on docker. Damn that you cant reproduce...

[Joan R.]

I try to upload the original and the corrupt one later...

[Joan R.]

Here is a link to dropbox, which contains the original file and the file downloaded (corrupted one) with the code from github.

Greets Joan

[Tim Fox]

I think you need to provide a full working reproducer, not just some snippets...

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/b92727a3-2388-4d21-bdf4-e0bdd92d8071%40googlegroups.com.

[Joan R.]

I posted a link to a full project which contains a application you can deploy. 

i cant install nginx on your machines ;)

Here is the  link to the  project on github.

Greets Joan

[Tim Fox]

I can't seem to run this... looks like the fatjar isn't being created properly.

Could you fix it / provide instructions for running?

Greets Joan

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/106b7199-5590-440f-849c-9e3366cd0eba%40googlegroups.com.

[Joan R.]

Hey Tim, thanks for your help!

As i wrote before, you need to adjust the variable "url" in the class "DownloadVerticle" to point to a url served by nginx. 

And you need to adjust the  variable "filePath" also in the class "DownloadVerticle" (a path where the file should be stored).

Then compile the code with:

mvn clean install

Then start the app

vertx run com.movingimage24.MainVerticle -cp vertx-pump-issue-1.0-SNAPSHOT-fat.jar

If you cant find it, search for:

String url = "http://vmpro.mi24.dev/ffh-1-nginx/vertxissue/Testimonial_Thoma.wmv";

final String filePath = File.separator + "opt/vertx-module" + File.separator + UUID.randomUUID().toString(); 

[Tim Fox]

On 10/08/15 14:41, Joan R. wrote:

Hey Tim, thanks for your help!

As i wrote before, you need to adjust the variable "url" in the class "DownloadVerticle" to point to a url served by nginx. 

And you need to adjust the  variable "filePath" also in the class "DownloadVerticle" (a path where the file should be stored).

Then compile the code with:

mvn clean install

Then start the app

vertx run com.movingimage24.MainVerticle -cp vertx-pump-issue-1.0-SNAPSHOT-fat.jar

Ah, I need to install vertx?

fatjars don't require vert.x to be pre-installed (it just makes things easier to run)

If you cant find it, search for:

String url = "http://vmpro.mi24.dev/ffh-1-nginx/vertxissue/Testimonial_Thoma.wmv";

final String filePath = File.separator + "opt/vertx-module" + File.separator + UUID.randomUUID().toString(); 

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/77f995f7-fa11-4169-be25-6b29690889ad%40googlegroups.com.

[Joan R.]

yes, sorry...

we deploy a jar to vertx without the vertx dependencies inside the jar. its necessary for our build system here...

[Tim Fox]

On 10/08/15 14:46, Joan R. wrote:

yes, sorry...

we deploy a jar to vertx without the vertx dependencies inside the jar. its necessary for our build system here...

got it. calling it a fatjar just threw me a bit as it's a standard jar.

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/25020f11-5975-4327-b422-3473826e51a8%40googlegroups.com.

[Tim Fox]

On 10/08/15 14:41, Joan R. wrote:

Hey Tim, thanks for your help!

As i wrote before, you need to adjust the variable "url" in the class "DownloadVerticle" to point to a url served by nginx. 

And you need to adjust the  variable "filePath" also in the class "DownloadVerticle" (a path where the file should be stored).

I'm not sure I understand why I need to setup nginx. Doesn't this issue manifest with any file downloaded or does it only occur with your specific file on nginx?

Then compile the code with:

mvn clean install

Then start the app

vertx run com.movingimage24.MainVerticle -cp vertx-pump-issue-1.0-SNAPSHOT-fat.jar

If you cant find it, search for:

String url = "http://vmpro.mi24.dev/ffh-1-nginx/vertxissue/Testimonial_Thoma.wmv";

final String filePath = File.separator + "opt/vertx-module" + File.separator + UUID.randomUUID().toString(); 

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/77f995f7-fa11-4169-be25-6b29690889ad%40googlegroups.com.

[Joan R.]

the file is not corrupt if i donwload a file from the internet (like this one). 

but if i dowload this file from nginx which (runs on my machine), the file is corrupt.

perhaps because its much faster ?

[Tim Fox]

I will take a closer look (I haven't run it yet), but at first glance a few observations:

1. it looks like you're not closing the async file once the response is complete. I would expect to see an asyncFile.close in the end handler of the HttpClientResponse
2. The pause and resume of the HttpClientResponse seem redundant.
3. Separate createFile and open are unnecessary. open can create the file too.
4. The url parsing seems unnecessarily complex and redundant.

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/9c5f91c6-5605-478c-b13f-3de6d95a3bd4%40googlegroups.com.

[Joan R.]

Hey tim!

As you can see in this post, Nat told me that the pump does that. it also fails when i closed the file.

pause was in vertx 2.X necessary so i dont start reading bytes before i registered my handlers.

yes i can create and open the file with one method call, but that should lead to a corrupt file.

yes, parsing the url is a bit complex, because the code is from a more complex application and that should also not lead to a corrupt file

Greets from berlin,

Joan

[Tim Fox]

On 10/08/15 17:09, Joan R. wrote:

Hey tim!

As you can see in this post, Nat told me that the pump does that.

pump does not close the file

it also fails when i closed the file.

pause was in vertx 2.X necessary so i dont start reading bytes before i registered my handlers.

no, it wouldn't be necessary in Vert.x 2 either.

yes i can create and open the file with one method call, but that should lead to a corrupt file.

why should it lead to a corrupt file?

yes, parsing the url is a bit complex, because the code is from a more complex application and that should also not lead to a corrupt file

Greets from berlin,

Joan

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/2c920f53-353b-4f77-a714-67d53446c438%40googlegroups.com.

[Nat]

Tim,

I guess the bug has something to do with pause()/resume() logic inside HttpClientResponseImpl.java. It's possible that when the resume() occurs, all the chunks were drained and queue behind the event loop. However, when one of handleChunk() methods is called, the write queue is full again so the chunks from the previous drain got enqueued into pausedChunks again. However, Netty could interrupt the event loop in between and enqueue new chunk from the connection. Thus, you end up with pauseChunks with shuffled chunks.

[Tim Fox]

pause/resume shouldn't change the order of the packets. if it does that would be a bug.

but this should be easy for Joan to verify - simply remove the pause/resume and see if the problem disappears... Joan?

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/036b67ad-634b-4555-b3d2-a64a0ae1b559%40googlegroups.com.

[Nat]

I mean pause/resume done internally by Pump. Not the one Joan has in the code. Also based on how the code looks like, it is also possible that endHandler will be called before all the buffers are consumed.

[Tim Fox]

On 10/08/15 17:35, Nat wrote:

I mean pause/resume done internally by Pump. Not the one Joan has in the code. Also based on how the code looks like, it is also possible that endHandler will be called before all the buffers are consumed.

I don't see how, Vert.x won't call the end handler until all buffers have been provided to the handler

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/8fa6abd8-6e92-48ee-8f26-44e8058e9b23%40googlegroups.com.

[Nat]

I refer to the code here:

private void doResume() {
  if (pausedChunks != null) {
    Buffer chunk;
    while ((chunk = pausedChunks.poll()) != null) {
      final Buffer theChunk = chunk;
      vertx.runOnContext(new VoidHandler() {
        @Override
        protected void handle() {
          handleChunk(theChunk);
        }
      });
    }
  }
  if (hasPausedEnd) {
    final LastHttpContent theTrailer = pausedTrailer;
    vertx.runOnContext(new VoidHandler() {
      @Override
      protected void handle() {
        handleEnd(theTrailer);
      }
    });
    hasPausedEnd = false;
    pausedTrailer = null;
  }
}

all the chunks are enqueued to be called asynchronously. During that period, autoRead has been set to true. When the task time quota has run out, Netty will steal some CPU cycles to process IO event and it might read LastHttpContent and schedule the call for handleEnd(). However, during that time period, some chunks filling up the write queue (in AsyncFile) again and get enqueued into pauseChunks. Therefore, it could be that handleEnd() is called before all the chunks were drained.

[Tim Fox]

On 10/08/15 17:48, Nat wrote:

I refer to the code here:

private void doResume() {
  if (pausedChunks != null) {
    Buffer chunk;
    while ((chunk = pausedChunks.poll()) != null) {
      final Buffer theChunk = chunk;
      vertx.runOnContext(new VoidHandler() {
        @Override
        protected void handle() {
          handleChunk(theChunk);
        }
      });
    }
  }
  if (hasPausedEnd) {
    final LastHttpContent theTrailer = pausedTrailer;
    vertx.runOnContext(new VoidHandler() {
      @Override
      protected void handle() {
        handleEnd(theTrailer);
      }
    });
    hasPausedEnd = false;
    pausedTrailer = null;
  }
}

all the chunks are enqueued to be called asynchronously. During that period, autoRead has been set to true. When the task time quota has run out, Netty will steal some CPU cycles to process IO event and it might read LastHttpContent and schedule the call for handleEnd(). However, during that time period, some chunks filling up the write queue (in AsyncFile) again and get enqueued into pauseChunks. Therefore, it could be that handleEnd() is called before all the chunks were drained.

Maybe you could break this down in more detail? I don't follow the logic here.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/1355b72f-65e9-4e3a-8800-2413ebe56033%40googlegroups.com.

[Joan R.]

I will remove the pause/resume Calls Later. And Post the result. But i am very sure it was necessary in vertx 2... It took us some time to figure that out. I cant find the stackoverflow link...

Thanks for your help!

[Tim Fox]

Please also add the code that closes the file and update the repo so I
can take a look :)

[Joan R.]

Hey Nat and Tim!

I am sorry to say that, but the files are still corrupt.

I updated the code on github.

Greets Joan

[Tim Fox]

I think you're going to have to debug this one yourself.

Without being able to reproduce this, I'm not sure there is much I can do.

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/284366de-322a-45e7-ada3-12d5bff14a89%40googlegroups.com.

[Nat]

Joan,

Does the file always corrupt at the same place? Can you try to call AsyncFile.setWriteQueueMaxSize(64 * 1024 * 1024); before you start the pump to see whether the problem disappear or not.

[Joan R.]

Yessa!

you are right. the file is correct now!

can i use this code in production or is it using a lot of memory or so ? 

[Tim Fox]

The underlying issue still needs to be fixed. The key to this will be in creating a reliable reproducer.

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/9e0b1a6a-3a11-4ca2-984f-f8a48250bc98%40googlegroups.com.

[Nat]

What happened if you change that number to be very small i.e. 1024? Does the file become very corrupted?

[Joan R.]

i dont know what i can do else.

if you using nginx + using my code on github + the original file i uploaded to dropbox, you cant reproduce it ?

what can i do else, i dont get it.

[Joan R.]

i must go to bed :)

i send an update tomorrow morning.

Greets Joan

[Alexander Lehmann]

When comparing the files in chunks, it looks like most of the file is correct, but inbetween some parts are different.

I tried to do this by splitting the files into chunks of 1k and calculating the md5 of the chunks to compare the lists with sdiff.

When searching for a continuous block of 1k chunks that are wrong, it looks like the checksums appear at a different offset in the file (e.g. 48k further on), so I would guess that the chunks are somehow switched in sequence.


On Monday, August 10, 2015 at 10:21:41 AM UTC+2, Joan R. wrote:

Here is a link to dropbox, which contains the original file and the file downloaded (corrupted one) with the code from github.

Greets Joan

[Alexander Lehmann]

I have tried the project with the original file on a ubuntu with virtualbox (which is about as close as I can get to your config, I guess) and the file is written correct, so I am at a loss how to reproduce the issue.

[Tim Fox]

On 10/08/15 19:52, Joan R. wrote:

i dont know what i can do else.

Try debugging the code. Figure out what is causing the underlying issue (Maybe Nat's hunch is correct?).

Once you understand why the file is getting corrupt, it should be possible to create a small test that reproduces the problem.

Then the actual problem can be fixed. That's exactly what I would do if I could reproduce it.

if you using nginx + using my code on github + the original file i uploaded to dropbox, you cant reproduce it ?

what can i do else, i dont get it.

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/4b1dd5cb-69e7-43a3-8a6d-b74cba34ea5c%40googlegroups.com.

[Joan R.]

To Nat: If i use 1024 for setWriteQueueMaxSize, its (not surprisingly) also corrupt. 

To be honest, i dont have a good diff tool for mac. 

So i cant tell you if the file created with a setWriteQueueMaxSize of 1024 is more corrupt then the file with the default settings.

Perhaps Alexander Lehmann can have a look to these two files.

I uploaded these corrupt files into the dropbox.

If anybody has a good binary diff tool, youre welcome. I use this one, but its not working good (crashes often...).

I still dont understand why you cant reproduce it. We can reproduce it on three different machines (2*OSX, coreOs+Docker).

If i have time, i try to debug it, but i dont know when i have time for that during normal work load. 

Greets and thanks, 

Joan

[Tim Fox]

I've added some diagnostic information in Vert.x which will tell if packets are getting unordered.

Please build Vert.x from this branch https://github.com/eclipse/vert.x/tree/httpclientresponsepump

And run your reproducer again. It will produce some output on stdout. Please post that here.

Thanks.

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/3e1ca0a5-f67e-415d-8cc6-4a72e046174d%40googlegroups.com.

[Nat]

private void doResume() {
  vertx.runOnContext(new VoidHandler() {

    if (pausedChunks != null) {
      Buffer chunk;
      while ((chunk = pausedChunks.poll()) != null) {
        final Buffer theChunk = chunk;

        handleChunk(theChunk);

        if (paused) {

          break;

        }
      }

    }
    if (!paused && hasPausedEnd) {

      final LastHttpContent theTrailer = pausedTrailer;

      handleEnd(theTrailer);

      hasPausedEnd = false;
      pausedTrailer = null;

    }

  });
}

Tim,

If the code looks something like above, it should address the issue.

[Tim Fox]

On 11/08/15 15:58, Nat wrote:

private void doResume() {
  vertx.runOnContext(new VoidHandler() {

    if (pausedChunks != null) {
      Buffer chunk;
      while ((chunk = pausedChunks.poll()) != null) {
        final Buffer theChunk = chunk;
        handleChunk(theChunk);

        if (paused) {

          break;

        }
      }

    }
    if (!paused && hasPausedEnd) {
      final LastHttpContent theTrailer = pausedTrailer;
      handleEnd(theTrailer);
      hasPausedEnd = false;
      pausedTrailer = null;
    }

  });
}

Tim,

If the code looks something like above, it should address the issue.

I would like to determine what the issue is first, before coming up with solutions :)

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/4013a707-6a92-4029-8fe3-fc29687527bc%40googlegroups.com.

[Nat]

Shoot first. Ask question later. ;-)

[Alexander Lehmann]

Actually I have not found a good tool to compare binary files either, I ended up creating chunks and using the md5sum checksum of each chunk to compare the files with diff -y.

When I check the different lines using 4k chunks against the original file, I get the following counts:

corruptedFile.wmv 1130
default.wmv 805
64_1024_1024.wmv 12008

the complete file consists of 12010 chunks, so in the last file only the first 2 chunks are correct.

the shell script I have used to create the checksum files is this, if you want to try it https://gist.github.com/alexlehm/c8ff69cae1950d7c0985

[Hemal Shah]

Hi Guys,

I have encountered very similar problem as described in this thread by Joan. I am using "2.1.4 (built 2014-10-23 10:34:33)" and recently we had to introduce flow control logic (pause and resume as used in Pump) to support large file uploads and downloads. Without, flow control logic we could not make upload and download of large file work reliably as expected. After adding flow control logic we started seeing good improvement in upload and download of large files. But, at times we have started seeing weird behavior on file download (small files especially less then 1MB) that file get's corrupted. File size is exactly same as we upload but while trying to open the file it complains about file is corrupted. And this behavior is not consistent. Meaning, if I try to download the file again it might download fine and will open up fine without any issues.

Any further update on this issue will be highly appreciated.

Joan,

Do you really don't see any issues after you bumped up MaxQueueSize limit to 64*1024*1024 ? And any idea how big files are you dealing with?

Thanks,
Hemal

[Alexander Lehmann]

The example file is about 48mb, so it should fit completely into the queue with 64mb queue size

[Tim Fox]

Ping :)

[Nat]

:-) I think Alex's result seems to indicate that the packets were shuffled.

[Tim Fox]

Please bear with me.

It's painful debugging this remotely. but we need to go through this process to isolate the issue.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/71c58ebe-748e-440d-a616-2c583d4a5241%40googlegroups.com.

[Nat]

As Joan is probably sleeping, I managed to find a nice short repro.

import io.vertx.core.Vertx;
import io.vertx.core.VertxOptions;
import io.vertx.core.buffer.Buffer;
import io.vertx.core.http.*;

import java.util.Arrays;
import java.util.Random;

public class Main {
    private static Vertx vertx;
    private static Buffer buffer;
    private static Buffer readBuffer;

    public static void main(String[] args) {
        VertxOptions o = new VertxOptions();
        vertx = Vertx.vertx(o);
        byte[] data = new byte[64 * 1024 * 1024];
        new Random().nextBytes(data);
        buffer = Buffer.buffer(data);
        readBuffer = Buffer.buffer(64 * 1024 * 1024);
        HttpServer httpServer = vertx.createHttpServer();
        httpServer.requestHandler(Main::handleWrite);
        httpServer.listen(10000);
        HttpClient httpClient = vertx.createHttpClient();
        HttpClientRequest clientRequest = httpClient.get(10000, "localhost", "/");
        clientRequest.handler(x -> {
            x.handler(y -> handleRead(x, y));
            x.endHandler(Main::handleDone);
        });

        clientRequest.end();
    }

    private static void handleDone(Void aVoid) {
        byte[] expectedData = buffer.getBytes();
        byte[] actualData = readBuffer.getBytes();
        System.out.println(Arrays.equals(expectedData, actualData));
    }

    private static void handleWrite(HttpServerRequest request) {
        request.response().setChunked(true);
        for (int i = 0; i < buffer.length() / 8192; i++) {
            request.response().write(buffer.slice(i * 8192, (i + 1) * 8192));
        }
        request.response().end();
    }

    private static void handleRead(HttpClientResponse response, Buffer buffer) {
        readBuffer.appendBuffer(buffer);
        for (int i = 0; i < 64; i++) {
            vertx.setTimer(1, n -> {
                try {
                    Thread.sleep(0);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            });
        }

        response.pause();
        vertx.setTimer(1, n -> response.resume());
    }
}

[Tim Fox]

Thanks,

I assumed you and Joan were running in verticles, if you're running embedded this makes a lot more sense.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/2377fada-13ac-4306-95ac-7f65298db846%40googlegroups.com.

[Nat]

What do you mean?

[Nat]

Tim,

Did you get a chance to take a look?

[Tim Fox]

I'm looking at it now.

The problem seems to be that Netty always prioritises IO events over other events (such as events submitted using vertx.runOnContext()), which can result in chunks being reordered in the case of pause/resume.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/69f6dd8f-9a55-4897-af8c-21179a472785%40googlegroups.com.

[Nat]

Correct. If there are more than 64 tasks sitting in the queue and it takes more than 1 ms by default, it will stop running the task and try to process IO instead. I think it's probably there to ensure that IO tasks has a chance to drain the queues.

[Joan R.]

Hey guys! 

Sorry for the delay! Damn work...

What can i do ? 

Should i test it again with the "special" vertx version?

@Hemal Shah: If i used 64*1024*1024, the file was playable without errors in vlc media player. 

 

[Tim Fox]

I don't know about that, but you can see that io events are *always* executed before waiting tasks here:

https://github.com/netty/netty/blob/4.0/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java#L349

https://github.com/netty/netty/blob/4.0/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java#L354

I.e. processSelectedKeys() is always executed before runAllTasks()

To make matters worse, if we call:

channel.setAutoRead(false);

There is no guarantee that some IO events won't come in afterwards - the semantics seem to mean "some time shortly after calling, no more IO events will be received".

All of this combined makes it hard for us to guarantee chunks are delivered to the user in the correct order, when we use vertx.runOnContext() to "replay" events.

Anyway, to cut a long story short, I've fixed it locally by concatenating paused chunks into a larger chunk which is delivered when the next chunk (or the last one) after the response has been resumed is delivered. That way we can avoid vertx.runOnContext().

There are a couple of other places in the code that seem to suffer from the same issue (e.g. in NetSocketImpl and possibly ServerConnection) so I need to fix those too.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/52e0e04a-17f2-483f-b763-e016c16b9896%40googlegroups.com.

[Joan R.]

@Net: Awesome, we finally have a repodurcer, thanks! :)

@Tim: What do you mean with "I assumed you and Joan were running in verticles, if you're running embedded this makes a lot more sense."?

I am using verticles which i deploy inside a vertx application. What do you mean with "embedded"? 

[Tim Fox]

embedded means "not running in verticles", as in Nat's example.

There are examples of embedded and not embedded in the examples repo.

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/4a094030-1dec-4db7-92fe-d8868c81bbd2%40googlegroups.com.

[Joan R.]

ah ok.

So it seems that its a bug in the deep inside of vertx/netty, so i cant do anything to help right?

[Tim Fox]

https://github.com/eclipse/vert.x/pull/1116

[Joan R.]

Great! As far as i can tell :)
When will this  fix be available in vertx? currently we are using curl to download the files... i want to change that as soon as possible

[Nat]

Based on the fix, I think it might still cause a problem in a rare occurrence where IO task is executed before the scheduled job. The right fix is not to turn autoRead back on unless all the paused chunks are drained.

public synchronized HttpClientResponse resume() {

if (paused) {

paused = false;

vertx.runOnContext(new VoidHandler() {

doResume();

}

}

}

public void doResume() {

if (pausedChunks != null) { Buffer chunk; while ((chunk = pausedChunks.poll()) != null) { final Buffer theChunk = chunk; handleChunk(theChunk); if (paused) { return; } } }

conn.doResume(); if (hasPausedEnd) { final LastHttpContent theTrailer = pausedTrailer; handleEnd(theTrailer); hasPausedEnd = false; pausedTrailer = null; }

[Tim Fox]

Could you elaborate a bit?

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/edf6d30a-a8e0-4b52-8499-c6b1fe855619%40googlegroups.com.

[Nat]

If the current executed task which calls into HttpClientResponseImpl.resume is the 64th task that got executed in the NioEventLoop and those 64 tasks have taken longer than the previous IO call, it will break out of line 363 and it will exit from runAllTasks(long) method and cause the IO selector epoll to be executed again. In which case, it will read the latest data before the old chunk get read by handleChunk() method.

Order of execution

1. HttpClientResponse.resume() is called.

2. HttpClientResponse.doResume() is called.

3. handleChunk is queued to be executed in the next event loop

4. ClientConnection.doResume() is called and it setup the selector for this connection

4. runAllTasks got a break because it is the 64th task and so far tasks have taken a significant amount of time

5. NioEventLoop.run() will continue the next loop at line 304

6. NioEventLoop.processSelectedKeys will be called and it will read the next chunk of data from ClientConnection

7. NioEventLoop. runAllTasks() will be called again.

8. At this point, the previous scheduled task to call handleChunk with previous paused chunks will be called

And there you go, you get yourself a re-ordered buffer.

...

[Tim Fox]

On 16/08/15 23:30, Nat wrote:

If the current executed task which calls into HttpClientResponseImpl.resume is the 64th task that got executed in the NioEventLoop and those 64 tasks have taken longer than the previous IO call, it will break out of line 363 and it will exit from runAllTasks(long) method and cause the IO selector epoll to be executed again. In which case, it will read the latest data before the old chunk get read by handleChunk() method.

Order of execution

1. HttpClientResponse.resume() is called.

2. HttpClientResponse.doResume() is called.

3. handleChunk is queued to be executed in the next event loop

We only do this if hasPausedEnd = true, and this is only true if the last chunk of the response has been received.

In normal pause/resume before the end of the response we do not call runOnContext() at all any more.

--

You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/6a0b3311-e3d3-4f48-a172-079e3ce73853%40googlegroups.com.

[Nat]

Ah I see. That would work as well.

On Sunday, August 16, 2015 at 11:52:17 PM UTC-7, Tim Fox wrote:

On 16/08/15 23:30, Nat wrote:

If the current executed task which calls into HttpClientResponseImpl.resume is the 64th task that got executed in the NioEventLoop and those 64 tasks have taken longer than the previous IO call, it will break out of line 363 and it will exit from runAllTasks (long) method and cause the IO selector epoll to be executed again. In which case, it will read the latest data before the old chunk get read by handleChunk () method.

Order of execution

1. HttpClientResponse.resume () is called.

2. HttpClientResponse.doResume () is called.

3. handleChunk is queued to be executed in the next event loop

WE only do this if hasPausedEnd = true, and this is only true if the Last chunk of the Response has Been Received. In normal PAUSE / resume before the End of the Response WE do not Call runOnContext () at any More All.

4. ClientConnection.doResume () is called and it setup the selector for this connection

4. runAllTasks got a break because it is the 64th task and so far tasks have taken a significant amount of time

5. NioEventLoop.run () will continue the next loop at line 304

6. NioEventLoop. processSelectedKeys Will be Called and it Will Read the Next chunk of Data from ClientConnection

7. NioEventLoop. RunAllTasks () will be called again.

8. At this point, the previous scheduled task to call handleChunk with previous paused chunks will be called

And there you go, you get yourself a re-ordered buffer.

On Sunday, August 16, 2015 at 12:51:21 AM UTC-7, Tim Fox wrote:

Could you Elaborate a bit? On 16/08/15 6:10, Nat wrote:

Based on the fix, I think it might still cause a problem in a rare occurrence where IO task is executed before the scheduled job. The right fix is ​​not to turn autoRead back on unless all the paused chunks are drained.

public synchronized HttpClientResponse resume () {

if (paused) {

paused = false ;

VertX . runOnContext ( new VoidHandler () {

doResume ();

ʱ??

ʱ??

ʱ??

public void doResume () {

if (pausedChunks = null!) {Buffer chunk; while ((Chunk = pausedChunks.poll ())! = Null) {final Buffer theChunk = chunk; handleChunk (theChunk); if (Paused) {return;}}}

conn.doResume (); if (hasPausedEnd) {final LastHttpContent theTrailer = pausedTrailer; handleEnd (theTrailer); hasPausedEnd = false; pausedTrailer = null;}

ʱ??

On Saturday, August 15, 2015 at 1:43:10 AM UTC-7, Tim Fox wrote:

https://github.com/eclipse/ vert.x / pull / 1116

On 15/08/15 08:34, Tim Fox wrote:

That I do not know about, BUT you CAN See That IO Events are * Always * Executed before Waiting tasks here: https://github.com/netty/ Netty / BLOB / 4.0 / Transport / src / main / Java / IO / Netty / Channel / nio / NioEventLoop.java # L349 https://github.com/netty/ Netty / BLOB / 4.0 / Transport / src / main / Java / IO / Netty / Channel / nio / NioEventLoop.java # L354 Ie processSelectedKeys () is Always Executed before runAllTasks () To make Matters worse, if WE Call: channel.setAutoRead (false); There is no Guarantee That some IO Events Will not Come in afterwards - the semantics SEEM to Mean "some time shortly after calling, no More IO Events Will be Received. " All of this Combined Makes it Hard for US to Guarantee chunks are delivered to the user in the correct Order, When WE Use vertx.runOnContext () to "Replay" Events. Anyway, to cut a long story short, I've fixed it locally by concatenating paused chunks into a larger chunk which is delivered when the next chunk (or the last one) after the response has been resumed is delivered. That way we can avoid vertx. runOnContext (). There are a couple of other places in the code That SEEM to suffer from the Same issue (eg in NetSocketImpl and Possibly ServerConnection) so I need to FIX Those TOO. On 14/08/15 16:27, Nat wrote ʱ??

Correct. If there are more than 64 tasks sitting in the queue and it takes more than 1 ms by default, it will stop running the task and try to process IO instead. I think it's probably there to ensure that IO tasks has a chance to . Drain the queues On Friday, August 14, 2015 at 7:35:26 AM UTC-7, Tim Fox wrote:

I'm looking at it now. The problem Seems to be That Netty Always prioritises IO Events over other Events (SUCH Events as submitted using vertx.runOnContext ()), Which CAN result in chunks being reordered in the Case of PAUSE / resume. On 14/08/15 15:13, Nat wrote:

Tim,

? Did you get a Chance to Take a look On Thursday, August 13, 2015 at 11:11:59 AM UTC-7, Nat wrote:

What do you Mean? On Thursday, August 13, 2015 at 11:09:55 AM UTC-7, Tim Fox wrote:

Thanks, I ASSUMED you and Joan Were running in verticles, if you're running this Embedded Makes a Lot More Sense. On 13/08/15 6:46 p.m., Nat wrote:

As Joan is probably sleeping, I managed to find a nice short repro.

Import io.vertx.core.Vertx ;
 Import io.vertx.core.VertxOptions ;
 Import io.vertx.core.buffer.Buffer ;
 Import io.vertx.core.http *. ;
 
Import java.util.Arrays ;
 Import Java. util.Random ;
 
public class Main {
     Private static VertX VertX ;
     Private static Buffer Buffer ;
     Private static Buffer ReadBuffer ;
 
    public static void main (String [] args) {
        VertxOptions O = new VertxOptions () ;
         VertX = VertX. VertX (O) ;
         byte [] Data = new byte [ 64 * 1024 * 1024 ] ;
         new Random () nextBytes (Data). ;
         Buffer . = Buffer Buffer (Data) ;
         ReadBuffer = Buffer. Buffer ( 64 * 1024 * 1024 ) ;
         HttpServer HTTPServer = VertX .createHttpServer () ;
         httpServer.requestHandler ( Main :: handleWrite ) ;
         httpServer.listen ( ten thousand ) ;
         HttpClient httpClient = VertX .createHttpClient () ;
         HttpClientRequest ClientRequest = httpClient.get ( 10 thousand , "localhost" , "/" ) ;
         clientRequest.handler (x -> {
            x.handler (y -> handleRead (x , y)) ;
             x.endHandler (Main :: handleDone ) ;
         }) ;

         clientRequest.end () ;
     }

    Private static void handleDone (Void Avoid) {
         byte [] expectedData = Buffer .getBytes () ;
         byte [] actualData = ReadBuffer .getBytes () ;
         System. out .println (Arrays. EQUA LS (expectedData , actualData)) ;
     }

    Private static void handleWrite (HttpServerRequest Request) {
        request.response () setChunked (. true ) ;
         for ( int i = 0 ; i < Buffer .length () / 8192 ; i ++) {
            request.response () write (. buffe r .slice (i * 8192 , (i + 1 ) * 8192 )) ;
         }
        request.response () End (). ;
     }

    Private static void handleRead (HttpClientResponse Response , Buffer Buffer) {
         ReadBuffer .appendBuffer ( Buffer) ;
         for ( int i = 0 ; i < 64 ; i ++) {
             VertX .setTimer ( 1 , N -> {
                 try {
                    Thread. sleep ( 0 ) ;
                 } catch (InterruptedException e) {
                    e.printStackTrace () ;
                 }
            }) ;
         }

        response.pause () ;
         VertX .setTimer ( 1 , N -> response.resume ()) ;
     }
ʱ??

On Wednesday, August 12, 2015 at 9:59:47 AM UTC-7, Tim Fox wrote:

Please Bear with Me. It's Painful debugging remotely this BUT WE need to go through this Process to isolate the issue.. On 12/08/15 17:42, Nat wrote:

:-) I think Alex's result seems to indicate that the packets were shuffled.

On Wednesday, August 12, 2015 at 9:04:44 AM UTC-7, Tim Fox wrote:

Ping :) On 11/08/15 15:17, Tim Fox wrote:

I've added some diagnostic information in vert.x Which Will Tell if packets are getting unordered. Please Build vert.x from this Branch https://github.com/eclipse/ vert.x / Tree / httpclientresponsepump And run your reproducer Again ... It Will Produce some output on stdout That Please post here . Thanks On 11/08/15 08:44, Joan R. wrote:

To Nat: If i use 1024 for setWriteQueueMaxSize, its (not surprisingly) also corrupt. 

To be honest, i dont have a good diff tool for mac. 

So i cant tell you if the file created with a setWriteQueueMaxSize of 1024 is more corrupt then the file with the default settings.

Perhaps Alexander Lehmann can have a look to these two files.

I Uploaded THESE Corrupt Files into the Dropbox .

If anybody has a good binary diff tool, youre welcome. I use this

ʱ??

[Joan R.]

Hey guys!

Again my question, when will this fix be available ?

[Tim Fox]

On 18/08/15 09:17, Joan R. wrote:

Hey guys!

Again my question, when will this fix be available ?

The fix is available now in master, if you want it now, you can build it yourself (very easy)

It will also be in the next release of Vert.x. We'll probably do a maintenance release of Vert.x 3.0.1 in the next couple of weeks.

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at http://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/977837ba-4702-4ad1-9198-ad54c43d623d%40googlegroups.com.

[Hemal Shah]

Hi Tim,

Is it possible to make this fix available in prior version of Vertx for eg: (2.1.4). As we planning to move to production with Vertx 2.1.4 very soon and we don't have any immediate plans to move to Vertx 3.x, therefore it will be helpful if you can provide fix in 2.x as well ?

Thanks,
Hemal

[Tim Fox]

It's already fixed in the 2.x branch and will be in the next 2.x release (2.1.7)

BTW 2.1.4 is pretty old, the latest release on the 2.x branch is 2.1.6.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/d889d145-954d-4713-8f0d-87b5c2b99b96%40googlegroups.com.

[Tim Fox]

On 18/08/15 20:43, Tim Fox wrote:

It's already fixed in the 2.x branch and will be in the next 2.x release (2.1.7)

Or you could build it yourself:

1. git clone https://github.com/eclipse/vert.x.git
2. cd vert.x
3. git checkout 2.x
4. ./gradlew clean dist

[Hemal Shah]

Any idea when 2.1.7 is getting released ?

Thanks,
Hemal

[Tim Fox]

Not for a while, we have to accumulate enough bugs to make it worthwhile, and there aren't many yet ;)

If you're in a hurry I suggest you build it yourself, it's really no different to what we'd release anyway.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/ea103f6a-392a-4d0f-8c49-cb19b152ab11%40googlegroups.com.

[Hemal Shah]

Ok. Thanks a lot!! Tim as always :)

[Hemal Shah]

While running command "./gradlew clean dist" as you mentioned above my build process is getting stuck on following line any idea ? I got this tried on one of my colleagues machine as well and he too had the same problem.

Download http://repo1.maven.org/maven2/com/google/javascript/closure-compiler/r1918/closure-compiler-r1918.jar
> Loading > 3.07 MB/3.20 MB downloaded

Thanks,
Hemal

[Tim Fox]

Not sure, but it may take some time the first time you do it as it has to download all the dependencies and gradle itself which has a load of dependencies.

Another possibility is network or proxy issues on your end.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/cd5fde24-472f-4be3-990f-dee0e2f051d6%40googlegroups.com.

[Hemal Shah]

Hi Tim,

Can you confirm that whether you applied fix for this issue in vertx-core/src/main/java/org/vertx/java/core/net/impl/ConnectionBase.java (under 2.x branch)?

And that's the only change was made to address corrupt files issue?

[Hemal Shah]

One more quick question..

To avail the fix of corrupted files on download is it possible I can keep referencing vertx-2.1.4 jar in my code base (in my pom.xml) and just upgrade vertx installation to vert.x-2.1.7. Will that be enough to do or I need to reference vert.x-2.1.7-SNAPSHOT in my code base as well?

[Hemal Shah]

Hi Tim,

Can you please reply to my below question? Only reason I am asking maybe such weird question is that I am not able to see working for me. Either, I am doing something wrong or fix is not really made.

Thanks!!