DJWassink / Promise-parallel-throttle

It's kinda like Promise.all(), but throttled!
MIT License
81 stars 3 forks source link

Throttle.all doesn't return on end #22

Closed bloodcarter closed 7 years ago

bloodcarter commented 7 years ago

I use it something like:

    //Function which should return a Promise
    const doCall = async (order) => {
        let res = await makeCall(order);
        logger.debug('makeCall (spi: %s) completed.', order.spi);  
        await promiseRetry(function (retry, number) {
          logger.debug('updateRow (spi: %s) attempt number %d', order.spi, number);  
          return updateRow(auth, res.rowNumber, res.comment, res.statusCode, res.operator)
          .catch(retry);
        });
        logger.debug('updateRow (spi: %s) completed.', order.spi);  
        await promiseRetry(function (retry, number) {
          logger.debug('writeNote (spi: %s) attempt number %d', order.spi, number);  
          return writeNote(auth, res.rowNumber, res.log, res.called_date)
          .catch(retry);
        });   
        logger.debug('writeNote (spi: %s) completed.', order.spi);  
        await promiseRetry(function (retry, number) {
          logger.debug('Rules.writeCallsHistory (spi: %s) attempt number %d', order.spi, number);  
          return Rules.writeCallsHistory(res, auth)
          .catch(retry);
        });                
        logger.debug('Rules.writeCallsHistory (spi: %s) completed.', order.spi);  
        return res;
    }
    //Queue with functions to be run
    const queue = new_orders.map(order => () => doCall(order));  

    const nextTaskCheck = (status, tasks) => {
      return new Promise((resolve, reject) => {
          resolve(status.amountStarted < tasks.length && Utils.isUtcOffsetInTimeRangeNow(7, 12, 18)); 
      });
  }

      await Throttle.all(queue, {
        maxInProgress: 5,
        nextCheck: nextTaskCheck
      });

The problem is that Throttle.al randomly doesn't return upon completition, it just hangs forever.

DJWassink commented 7 years ago

Not sure why it hangs tho. One reason I could come up with is the fact that your nextTaskCheck always returns a result. If the nextTaskCheck returns a falsey value it will not finish the task.

The actual way the nextTashCheck should be use is to wait till a condition has been met and then return a true value, so something like this:

    const nextTaskCheck = (status, tasks) => {
      return new Promise((resolve, reject) => {
          const interval = setInterval(() => {
               if (status.amountStarted < tasks.length && Utils.isUtcOffsetInTimeRangeNow(7, 12, 18)) {
                    clearInterval(interval);
                    resolve(true);
               }
          }, 100);
      });
  }

I know this is kinda ugly but for now I don't have an other solution in mind.

bloodcarter commented 7 years ago

Is there another way to stop execution of Throttle.all ?

ср, 25 окт. 2017 г. в 19:36, Dirk-Jan Wassink notifications@github.com:

Not sure why it hangs tho. One reason I could come up with is the fact that your nextTaskCheck always returns a result. If the nextTaskCheck returns a falsey value it will not finish the task.

The actual way the nextTashCheck should be use is to wait till a condition has been met and then return a true value, so something like this:

const nextTaskCheck = (status, tasks) => {
  return new Promise((resolve, reject) => {
      while(!(status.amountStarted < tasks.length && Utils.isUtcOffsetInTimeRangeNow(7, 12, 18))) {}
      resolve(true);
  });

}

I know this is kinda ugly and you shouldn't want to use a while loop (more something like a interval which can call the resolve and clean itself). But for now I don't have an other solution in mind.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/DJWassink/Promise-parallel-throttle/issues/22#issuecomment-339315672, or mute the thread https://github.com/notifications/unsubscribe-auth/APyuxoqnqjSrD9tw-RjV-BLMfTvKPPr3ks5svytjgaJpZM4P5rL9 .

-- --Vladislav Chernyshov Co-founder @ Dedal http://beta.ded.al https://medium.com/dedal

DJWassink commented 7 years ago

Sadly no.

If you have any ideas on a workable solution or how the api to achieve this should look like feel free to let me know.

bloodcarter commented 7 years ago

Ok, so how do I correctly stop execution using nextTaskCheck? I thought that if it resolves true next task is being started, if it resolves false, then the execution eventually stops . Right? Or you're saying that it should not resolve anything in order to stop or what?

ср, 25 окт. 2017 г. в 22:14, Dirk-Jan Wassink notifications@github.com:

Sadly no.

If you have any ideas on a workable solution or how the api to achieve this should look like feel free to let me know.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/DJWassink/Promise-parallel-throttle/issues/22#issuecomment-339364355, or mute the thread https://github.com/notifications/unsubscribe-auth/APyuxvbQ9RtE1wJNb4MDYNayW3skjT_Vks5sv1BWgaJpZM4P5rL9 .

-- --Vladislav Chernyshov Co-founder @ Dedal http://beta.ded.al https://medium.com/dedal

DJWassink commented 7 years ago

If it resolves with true it indeeds starts the next task, however returning false should indeed eventually stop the whole chain of tasks running.

I will take a better look at it why returning false isn't correctly working, I will get back on this.

DJWassink commented 7 years ago

All right I feel kinda stupid now :)

If you want to stop executing the whole chain of tasks simply rejecting the promise would stop everything check the following test case: https://github.com/DJWassink/Promise-parallel-throttle/blob/6c5ec1bcf2369d6c72009b9e9848e9f726f8b1a6/tests/throttle.test.ts#L193-L234

However if my memory serves me returning a falsey value would indeed let the chain hang since it is missing a completion of a task. This is indeed unwanted behavior and should be fixed.

However for your question how to abort the progress would be to reject the promise instead of resolving it.

bloodcarter commented 7 years ago

Hmm, but your default nextTaskCheck resolves false when amountDone >= queue.length. Why in this case resolving false is a correct behavior?

ср, 25 окт. 2017 г. в 22:42, Dirk-Jan Wassink notifications@github.com:

All right I feel kinda stupid now :)

If you want to stop executing the whole chain of tasks simply rejecting the promise would stop everything check the following test case:

https://github.com/DJWassink/Promise-parallel-throttle/blob/6c5ec1bcf2369d6c72009b9e9848e9f726f8b1a6/tests/throttle.test.ts#L193

However if my memory serves me returning a falsey value would indeed let the chain hang since it is missing a completion of a task. This is indeed unwanted behavior and should be fixed.

However for your question how to abort the progress would be to reject the promise instead of resolving it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/DJWassink/Promise-parallel-throttle/issues/22#issuecomment-339374141, or mute the thread https://github.com/notifications/unsubscribe-auth/APyuxvJSGGNA9QwhBvgmPV2NewrIi_CLks5sv1bngaJpZM4P5rL9 .

-- --Vladislav Chernyshov Co-founder @ Dedal http://beta.ded.al https://medium.com/dedal

DJWassink commented 7 years ago

aah right now I get where the confusion comes from 😄 the check I use myself isn't actually the one that anounces the end of a chain of tasks. When the amount of finished tasks are equal to the amount of tasks the throttle will resolve, thus not relaying on the nextCheck for this kind of behavior.

DJWassink commented 7 years ago

Sorry but I am having difficulty reproducing the issue, when returning a falsey value in the nextTaskCheck the sequence executes correctly on my machine, are you sure the issue isn't in the task itself? If not could you reproduce the issue in a format which I can copy and run?

bloodcarter commented 7 years ago

Hmm...maybe. What will be if a task throws an uncaught exception somewhere?

DJWassink commented 7 years ago

If the task throws an exception the Throttle will catch this exception, if however your task doesn't throw an exception bit still crashes (quite common with nested promises) the Throttle will never know that a task crashed, so it is necessary to always check if your tasks are correctly implemented and that there is no way the task could crash without throwing an exception to the outside world.

DJWassink commented 7 years ago

I'm sorry but if you can't give me a reproducible scenario I am not able to solve this issue, thus for now I am closing this issue.