Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

Reject the signing promises on failure. #1666

Merged
merged 1 commit into from
Sep 29, 2016

Conversation

fromkeith
Copy link
Contributor

@fromkeith fromkeith commented Sep 22, 2016

Brief description of the changes [REQUIRED]

If signing a file for s3 upload fails FineUploader hangs. Promises related to signing were not calling failure on their parent promises.

Repo steps:

  1. Find a large file.
  2. Put it in an SD card.
  3. Add the file to FineUploader
  4. Pull out SD card before the signing can finish.
  5. Signing methods fail, rejecting their promise.
  6. Promise rejection is not propagated up.

What browsers and operating systems have you tested these changes on? [REQUIRED]

Windows 10. Chrome

Are all automated tests passing? [REQUIRED]

Yes

Is this pull request against develop or some other non-master branch? [REQUIRED]

Yes. Develop.

@fromkeith fromkeith changed the title Reject a the signing promises on failure. Reject the signing promises on failure. Sep 22, 2016
@rnicholus
Copy link
Member

Thanks for the PR. I'm a bit surprised that this is broken. I seem to remember this working at some point. I can see how this would fail if the file suddenly disappeared during the upload process.

I'll take a closer look soon.

@rnicholus rnicholus added this to the 5.11.9 milestone Sep 22, 2016
@fromkeith
Copy link
Contributor Author

I know if the file is pulled before signing, it will have a size of 0 and I think the signing will complete succesfully. It just signs what it thinks an empty file. Then it uploads to s3, and of course s3 complains about an empty file. The root of that problem is that the file size went to zero when it wasn't zero to begin with. I couldn't figure out where to add code to solve that, so instead thought relying on S3s errors would be fine.
The error that I encountered, was the FileReader getting an error when creating the signature. It would properly reject its promise. However, the calls to getEncodedHashedPayload did not listen for the rejection. So I just went in and propagated up the rejection call.

@rnicholus
Copy link
Member

Interesting - thanks for the details. You are correct about the empty file when it is removed from the filesystem during the upload process. I'll take a closer look when I get a chance.

@rnicholus
Copy link
Member

I expect to be able to look at this closer tomorrow. If all looks well, I'll merge into develop as part of 5.11.9.

@rnicholus
Copy link
Member

Can you describe the symptoms of this issue? I'm not exactly sure what I should be seeing when attempting to reproduce.

@fromkeith
Copy link
Contributor Author

I saw the file just 'hang'... It never actually starts uploading or reports an error. No XHR requests get made and the state of the file doesn't change.

@rnicholus
Copy link
Member

rnicholus commented Sep 29, 2016

I'm not able to follow the exact steps in your description, but I did notice a couple related issues:

  1. When the network connection is pulled, the upload hangs for quite a while. I think this can be solved with an XMLHttpRequest timeout.
  2. If the file is deleted while the upload is in progress, fine uploader will continue to send chunks, but they are 0 sized. It seems like Fine Uploader should fail the upload in this case.

I think both of these issues should be addressed as part of this case. I'll look into making these adjustments soon. I hope to release those along with your fix to compete 5.11.9.

@fromkeith
Copy link
Contributor Author

If you put a break point on the 'failure' below, do you ever hit it?

getEncodedHashedPayload: function(body) {
    ...
        reader = new FileReader();
        reader.onloadend = function(e) {
            if (e.target.readyState === FileReader.DONE) {
                if (e.target.error) {
                    promise.failure(e.target.error); // here
                }
                else {
                    var wordArray = qq.CryptoJS.lib.WordArray.create(e.target.result);
                    promise.success(qq.CryptoJS.SHA256(wordArray).toString());
                }
            }
        };
   ...

I was able to hit the failure if I pulled the sd card while it was trying to read the file.

From what you saw:

  1. is something that is especially problematic for really bad internet connections. I would love to see a timeout setting somewhere.
  2. Definitely ran into this. Less useless xhr requests would be awesome

@rnicholus
Copy link
Member

I was never able to hit the breakpoint above. I simply saw FU send a bunch of empty chunks at S3 after the file was deleted. But I believe your changes are correct, so I will merge this in.

I'm going to prioritize #743 and #1669 to address the other two issues we discussed. If you'd like to help out with either, let me know.

@rnicholus rnicholus merged commit 2b7fa0b into FineUploader:develop Sep 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants