Replies: 10 comments 6 replies
-
Beta Was this translation helpful? Give feedback.
-
@yoavniran This looks great to me. Just a few comments & questions.
While item lifecycle events will not be called on pending items, will the
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.
I imagine we will configure
This seems fine to me. @mwdiaz Anything to add or correct on my comments? |
Beta Was this translation helpful? Give feedback.
-
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.
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 One thing I would like to add with respect to the |
Beta Was this translation helpful? Give feedback.
-
@mwdiaz @AJLemos Thanks for your input.
Of course.
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.
I will definitely look into that and fix (if possible)
👍 That sounds like a perfectly fine choice for your use-case I will start working on the implementation soon |
Beta Was this translation helpful? Give feedback.
-
Added new drawback (side-effect) of the suggested fast-abort (retry) |
Beta Was this translation helpful? Give feedback.
-
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. |
Beta Was this translation helpful? Give feedback.
-
Hi @yoavniran. Is there any update on this addition? Thanks for your time. |
Beta Was this translation helpful? Give feedback.
-
@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 |
Beta Was this translation helpful? Give feedback.
-
@yoavniran I've implemented 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. One thing I'll note is that we typically just store Here you can see we manually set the This doesn't appear to be an issue but I'm curious what you think about this approach?
|
Beta Was this translation helpful? Give feedback.
-
RFC released as part of 1.1.0 |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
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
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
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
Beta Was this translation helpful? Give feedback.
All reactions