RFC - A more performant abort-all mechanism for large count of queue items

[yoavniran]

Summary

As described in #381, when there are many items in the queue, calling abort-all will iterate every item in the queue and call abort on each in a (sync) loop. This can cause UI freeze and degraded user experience.

Motivation

• This is a problem as it will affect UI and certainly will degrade the app from sustaining 60fps.

• Uploady should not hurt the performance of the hosting application if it can be avoided

Detailed design

The following constraints are taken into consideration

1. The need to solve the performance issue
2. Don't impact existing implementations or try to minimize the impact as much as possible

Suggested solution is dynamic: From a certain item count, the more performant mechanism (pmode) will be used automatically. With opt-out possibility + threshold configuration

This way, (most) current implementations remain untouched (most dont reach 100+ of items at once + user aborts) while seamlessly switching to improved performance without need for intervention unless explicitly needing to turn off such behavior or modify its threshold.

Unless implementation specifically tracks item add vs. item finalize or doesnt need to show item status after abort all, it will not be aware of the change even if pmode is activated.

High Level

Identify already uploading vs pending

• Today, the Uploader iterates over every item to find its status. For uploading items, abort is called, cancelling the XHR. For pending items, it simply marks them as aborted.

• The new mode means the Uploader will work less in order to keep the UI responsive. However, it will still need to reach all currently uploading items and call their abort method. Since there is no other way to cancel a running XHR upload.

• Items/batches that haven't started should be removed without individually calling their respective events. This will ensure most items in the queue are discarded without iteration.

Technical Design

Uploader queue already holds a map of { item id : abort method }

• Once abort-batch or abort-all is called. The number of unfinished items will be matched to a new threshold configuration param (number)
• In case the count is equal/greater than threshold, the performant mode (pmode) will be chosen
• In case count is less than, current Uploader behavior will be chosen (no change)
• Iterating over just the already-uploading items is easy as Uploader can use abort map in queue and call abort directly on currently uploading items. Typically the number of actively uploading items is small.
• All pending items and respective batch(es) will be deleted from the queue without triggering any lifecycle events

• [Updated July 25, 2022] - Calling abort on a single item will not be affected by this change

Configuration

New UploadOptions Parameter:

Name: fastAbortThreshold
Type: Number
Default: 100
Opt Out: In case param value is 0 pmode will never be used
Min Value: 1
Max Value: Infinity

Drawbacks

• A side-effect of the new pmode will be that events will not be triggered for pending items. Only for started ones. For example, this will cause imbalance between the "Add" event and no "finish", "abort", etc. events. Implementations will need to take this into account.

• Having two behaviors can be confusing and feel unpredictable. Documentation is required to explain

• Default behavior does change for existing implementations (albeit unlikely to be felt) - does this require major version (probably not)

• [2022-08-01] Retry - the @rpldy/retry package relies on ABORT/ERROR batch item event to enable retry for these items. pmode abort means that retry will not be possible

[yoavniran]

@mwdiaz @AJLemos
Please review and comment

[AJLemos]

@yoavniran This looks great to me. Just a few comments & questions.

All pending items and respective batch(es) will be deleted from the queue without triggering any lifecycle events

A side-effect of the new pmode will be that events will not be triggered for pending items. Only for started ones. For example, this will cause imbalance between the "Add" event and no "finish", "abort", etc. events. Implementations will need to take this into account.

While item lifecycle events will not be called on pending items, will the useAllAbortListener() event listener still be called in this case? Giving the consumer a callback to update application state after abort all has been called.

Typically the number of actively uploading items is small.

This is true for our implementation. We currently have this limited to 3 concurrent uploads but may reduce this to 2 as we continue to work with QA. It is unlikely we'd increase this above 3.

Having two behaviors can be confusing and feel unpredictable. Documentation is required to explain

I imagine we will configure fastAbortThreshold to 1 so the same behavior is always used on abort all. This way we would only need to handle one callback behavior on abort all, via useAllAbortListener(), instead of handling two different behaviors depending on the number of pending items.

Default behavior does change for existing implementations (albeit unlikely to be felt) - does this require major version (probably not)

This seems fine to me.

@mwdiaz Anything to add or correct on my comments?

[mwdiaz]

I agree with @AJLemos's assessment, including that we would just set the threshold to 1 since there are other reasons besides performance that we don't want to invoke lifecycle methods in this case.

Having two behaviors can be confusing and feel unpredictable. Documentation is required to explain

This bit is kind of unfortunate, but I'm not sure if there's a better option? Obviously, the behavior could change to always use a fast abort approach, but that would require a major version bump and would also remove the ability for consumers to have an implementation that depends on lifecycle events for individual items in an abort all scenario. The only other approach I can think of at the moment is providing some indication on the BatchItem that is passed to useItemAbortListener() that would allow a callback to skip doing work when the item was aborted as part of an abort all; the implementation could then rely completely on useAllAbortListener() to do its state updates. This would still lead to potentially thousands of unnecessary function calls, so I'm not sure if performance could still be an issue.

One thing I would like to add with respect to the useAllAbortListener() lifecycle is something I've noticed is that when aborting a queue that contains pending uploads, the asynchronous nature of aborting the XHR connection causes those in progress items to complete their abort lifecycle (useItemAbortListener()) after the overall queue abort lifecycle (useAllAbortListener()). It might be nice if as part of this change we could also ensure that useAllAbortListener() is always called after all items are fully aborted.

[yoavniran]

@mwdiaz @AJLemos Thanks for your input.

...will the useAllAbortListener() event listener still be called in this case?

Of course.

...This would still lead to potentially thousands of unnecessary function calls, so I'm not sure if performance could still be an issue.

Exactly. I think the middle-ground approach here is best as Im betting on most implementations never really reaching the need to tackle both behaviors while still having control over the mechanism if they so choose.

...the XHR connection causes those in progress items to complete their abort lifecycle (useItemAbortListener()) after the overall queue abort lifecycle

I will definitely look into that and fix (if possible)

I imagine we will configure fastAbortThreshold to 1 so the same behavior is always used on abort all. This way we would only need to handle one callback behavior on abort all, via useAllAbortListener()...

👍 That sounds like a perfectly fine choice for your use-case

I will start working on the implementation soon
thanks!

[yoavniran]

Added new drawback (side-effect) of the suggested fast-abort (retry)

[AJLemos]

No retry functionality after an abort all action is a completely acceptable side-effect in my opinion.

Currently our upload UI does not allow retry on aborted items. We currently only use the retry functionality for an automatic retry attempt after receiving specific HTTP errors or specific backend error responses.

[AJLemos]

Hi @yoavniran. Is there any update on this addition? Thanks for your time.

[yoavniran]

Hey @AJLemos
I just managed to finish all of the required changes (PR #392)
Its been a slow process as I had very little time recently to focus on Uploady (busy time at work, life, etc.)

Right now, there are several specs failing so I need to fix them and make sure the changes arent detrimental for unforeseen reasons.

Once I have all of the changes committed Id like to create an RC version that you will be able to test with your code.
Ill let you know as soon as I manage that.

[mwdiaz]

Thanks for all the hard work, it's much appreciated.

[yoavniran]

@AJLemos @mwdiaz implementation based on this RFC is now merged and published as 1.1.0-rc.0

It will be great if you could test this version with your implementation and let me know how well it works

[AJLemos]

Thanks! I'll make time to test this version this week. Likely in the next few days.

[yoavniran]

Great. Notice that the latest RC is now: 1.1.0-rc.1

[AJLemos]

@yoavniran I've implemented fastAbortThreshold and abort all handling in useItemAbortListener() and so far this works fantastic! We set fastAbortThreshold to 1 so abort all handling is fully decoupled from abort item handling.

I've tested aborting file queues of 5,000 items and the "abort-all" event is rendered instantly (note we virtualize the file list, which helps render this many items quickly).

Our UI does not exposes file "batches" to users so I have not tested this functionality with the "abort-batch" events.

We have thorough unit test coverage on all our uploader code and I don't detect any regressions with this change. fastAbortThreshold also works as expected with the mock sender enhancer.

One thing I'll note is that we typically just store BatchItems returned from uploads's hooks in our application state unmodified, but the useAllAbortListener() does not return aborted BatchItems. This isn't a problem but it does mean that we have to "replicate" the aborted item state when users cancel all uploads.

Here you can see we manually set the state and uploadResponse properties on aborted BatchItems in our state reducer.

This doesn't appear to be an issue but I'm curious what you think about this approach?

  if (type === 'abortAll') {
    let allAbortedItems: FileListState = {};

    // `useAllAbortListener()` does not return an array of `BatchItem`s (like
    // most Uploady hooks do), so item `state` and `uploadResponse` must be
    // manually set to "aborted" when building the `abortedItems` list in order
    // to be displayed correctly in the `<FileList>` (or similar) component.
    // Setting these two properties matches Uploady's internal `BatchItem` state
    // for these items.
    Object.values({ ...activeItems, ...pendingItems  }).forEach((abortedItem) => {
      let { id } = abortedItem;

      allAbortedItems[id] = {
        ...abortedItem,
        state: FILE_STATES.ABORTED,
        uploadResponse: 'aborted',
      };
    });

    return {
      ...state,
      pendingItems: {},
      activeItems: {},
      abortedItems: { ...allAbortedItems, ...abortedItems  },
    };
  }

[yoavniran]

thats great to hear!

Regarding your reducer - looks good to me.
Because of the way things are handled internally I doubt allAbort event will expose items so its the right approach.

I will publish the 1.1.0 version as latest soon

[AJLemos]

Perfect. Thanks!

[yoavniran]

RFC released as part of 1.1.0