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

'attemptCount' is not working as intended on network error #66

Closed
Nerbeer opened this issue Oct 20, 2021 · 1 comment
Closed

'attemptCount' is not working as intended on network error #66

Nerbeer opened this issue Oct 20, 2021 · 1 comment

Comments

@Nerbeer
Copy link
Contributor

Nerbeer commented Oct 20, 2021

Hi!
I noticed some strange behaviour of retry attempts when request is failed due to network error and idk if it's intended or not :)
attemptCount is updated only when we receive response, but if we get network error or CORS error in my case, attemptCount is not changing and we keep spamming server with requests.
Shouldn't attempts control this case too?

/**
   * Manage the whole upload by calling getChunk & sendChunk
   * handle errors & retries and dispatch events
   */
  private sendChunks() {
    if (this.paused || this.offline || this.success) {
      return;
    }

    this.getChunk()
      .then(() => this.sendChunk())  <-------- maybe here?
      .then((res) => {
        this.attemptCount = this.attemptCount + 1;   <-------- Shouldn't it be somewhere else?

        if (SUCCESSFUL_CHUNK_UPLOAD_CODES.includes(res.statusCode)) {

        .....

      .catch((err) => {  <-------- sendChunk failed and skipped attemptCount++
        if (this.paused || this.offline) {
          return;
        }

        // this type of error can happen after network disconnection on CORS setup
        this.manageRetries();
      });
@mmcc
Copy link
Contributor

mmcc commented Oct 21, 2021

Hmm that's a good catch. Yes, probably makes sense to just move that increment into the block with sendChunk. Really I keep thinking we should move to tracking attempt count on individual chunks, but that's a much bigger change.

Want to submit a PR for this?

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

No branches or pull requests

3 participants