Limit inProgress using maxItems

[fathyb]

Blocked by #10

---

If the done callback is never called inProgress can grow infinitely when addItem is called. This PR checks that inProgress < maxItems before adding items to it.

---

Ref: https://segment.atlassian.net/browse/LIB-359
cc @f2prateek

[f2prateek]

This looks good to me.

But what do you think about making it so that (inProgress + queue.Length < max). I think that maybe a better guarantee to provide.

[f2prateek]

why the ++ in inProgressSize++?

[f2prateek]

oh it's because the loop adds to the inProgress map - makes sense!

[fathyb]

Because we add an item to inProgress inside of the loop, so the length grows. It's a slightly faster version of Well, looks like I don't have real-time comments 😅

[fathyb]

@f2prateek not really sure. To enforce it dynamically (based on the sum of the two queue sizes) you would need to drop new items, while the current behaviour is to drop the oldest item when maxItems is exceeded.

Using a fixed size for both makes queue behave as a back-pressure buffer for inProgress. Not sure if that makes sense so here is an example using each extremes (for maxItems=100):

• fixed queue size: push item to queue, drop first queue item if queue > maxItems

  • queue=99,inProgress=100: item queued
  • queue=0,inProgress=100: item queued
  • queue=100,inProgress=100: oldest queue item dropped, new item queued
  • queue=100,inProgress=0: oldest queue item dropped, new item queued

• dynamic queue size: if queue + inProgress < maxItems add item to queue

  • queue=49,inProgress=50: item queued
  • queue=0,inProgress=100: item ignored, we can't pop queue nor add an item
  • queue=50,inProgress=50: item ignored, you can workaround this on by poping queue
  • queue=100,inProgress=0: item queued

[f2prateek]

LGTM! Pending CI passes

[codecov-io]

Codecov Report

Merging #11 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #11      +/-   ##
==========================================
- Coverage    94.4%   94.38%   -0.03%     
==========================================
  Files           4        4              
  Lines         268      267       -1     
==========================================
- Hits          253      252       -1     
  Misses         15       15

Impacted Files	Coverage Δ
lib/index.js	95.27% <100%> (-0.04%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9eebb5e...78c309d. Read the comment docs.

[fathyb]

#10 needs to be merged first to fix the tests

[f2prateek]

@fathyb could you rebase this now that #11 is released?

[fathyb]

@f2prateek rebased, CI now passes 💯