muxinc / upchunk

Uploads Chunks! Takes big files, splits them up, then uploads each one with care (and PUT requests).
MIT License
336 stars 46 forks source link

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

Closed Nerbeer closed 2 years ago

Nerbeer commented 2 years ago

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 commented 2 years ago

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?