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

Permanent error should stop Flow from sending chunks. #35

Closed
nuvan opened this issue Jul 16, 2014 · 8 comments
Closed

Permanent error should stop Flow from sending chunks. #35

nuvan opened this issue Jul 16, 2014 · 8 comments
Labels

Comments

@nuvan
Copy link

nuvan commented Jul 16, 2014

HHi guys. First let me thank you all for this awesome lib.

I don't know if I'm interpreting this right but if a permanent error arouses shouldn't the chunks stop being sent to the server (using testChunks: true)?
I've added a CRC field using the preprocess method. I'm testing the CRC sent in every chunck at server side and replying with 409 if a chunk doesn't match, while one tries to resume a file. I've also added the 409 code as a permanent error.
The behaviour I'm noticing is that flow.js keeps sending chunks independently of the configured permanent errors.

Is this a issue or a misinterpretation of the feature?

@nuvan nuvan closed this as completed Jul 16, 2014
@nuvan
Copy link
Author

nuvan commented Jul 16, 2014

Meanwhile I've looked at the code and added this around line 1140-1154 to test the behaviour and it seems to solve the issue. Now if I receive a 409 (conflict) flow stops the upload process and marks it as an error.

this.testHandler = function(event) {
  var status = $.status();
  if (status === 'success') {
    $.tested = true;
    $.fileObj.chunkEvent(status, $.message());
    $.flowObj.uploadNextChunk();
  } else if (status === 'error') {
    console.log("Irrecoverable error");
    $.fileObj.chunkEvent(status, $.message());
  } else if (!$.fileObj.paused) {// Error might be caused by file pause method
    $.tested = true;
    $.send();
  }
};

(I've closed the issue by mistake)

@nuvan nuvan reopened this Jul 16, 2014
@AidasK
Copy link
Member

AidasK commented Jul 17, 2014

File testing does not support permanent error handling at the moment and this should be implemented.
Your code causes to fail one test case (File testing feature is not working anymore). If you could fix this, please make a pull request.

Line which is causing problems:
https://github.com/flowjs/flow.js/blob/master/src/flow.js#L1309
maxChunkRetries by default is set to 0 and this statement is always true then file is being tested.

@nuvan
Copy link
Author

nuvan commented Jul 18, 2014

Thanks for the reply @AidasK . I'll give it a stab this weekend.

@AidasK
Copy link
Member

AidasK commented Nov 22, 2014

Should be fixed with c2080ef
Please test it, will be released once tested in a production.

@AidasK AidasK closed this as completed Nov 22, 2014
@adiroiban
Copy link

Would be nice to also update the documentation/README to describe how this can be done. Thanks!

@adiroiban
Copy link

Do you/we want to abort on any errors?

For example 503 Service Unavailable is a transient server error and for this kind of error we would like to flow.js client to retry the test.

From this point of view, it would be nice if flow.js would allow configuring a list of error codes which has the semantic of "reject".

I would say that we would need a list of rejectErrorCode = [409] and stop testing only if this specific error code is returned... for all other errors, retry.

Thanks!

@AidasK
Copy link
Member

AidasK commented Nov 23, 2014

We have such an option, it is called permanentErrors https://github.com/flowjs/flow.js#configuration.

Unfortunately, request retry works only for upload requests. How file testing works:

  1. We send GET(by default) request to the server with chunk params
  2. Server checks and validates the file:
  • if file is invalid, server returns error code, which is in permanentErrors list, file upload stops.
  • if file is valid and file chunk exists on the server side, server returns status from successStatuses list. Client moves to the next chunk test/upload.
  • if file is valid and file chunk does not exists on the server side, server returns any status, which is not in the permanentErrors and successStatuses lists. Client uploads the chunk.

In order to support retry feature for file testing, we need a separate status list, such as chunkNotFoundStatuses. If response status is not in this array, then retry.

@adiroiban
Copy link

Many thanks! I think that copying your comment in the documentation would be a great improvement to the documentation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants