-
Notifications
You must be signed in to change notification settings - Fork 632
fix(lib-storage): improve concurrency and abortion management #7322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(lib-storage): improve concurrency and abortion management #7322
Conversation
5228307
to
3c15036
Compare
3c15036
to
68b910d
Compare
); | ||
} | ||
this.sent = true; | ||
return await Promise.race([this.__doMultipartUpload(), this.__abortTimeout(this.abortController.signal)]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The race here could cause the returned promise to be rejected when __doMultipartUpload
actually succeeds (and is still working on updating tags).
const currentUpload = this.__doConcurrentUpload(dataFeeder).catch((err) => { | ||
concurrentUploaderFailures.push(err); | ||
}); | ||
this.concurrentUploaders.push(currentUpload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was no point in keeping a reference on the instance as it was not used anywhere else.
* Optional abort signal for controlling this upload's abort signal externally. | ||
*/ | ||
abortController?: AbortController; | ||
abortSignal?: globalThis.AbortSignal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code technically supports IAbortSignal
but since those are kinda deprecated, and this is a new feature, there is no point in adding more legacy here.
Issue
#6426
Description
This change improves the following aspects of the lib:
abortSignal
instead of anabortController
(still available but deprecated) in order to be more consistent with other APIs. This also allows to useUpload
withAbortSignal.timeout()
Promise.race
would causedone()
to reject as soon as the abort controller is aborted, but would not wait for pending tasks to complete. This prevents user code from properly controlling concurrency. This change also avoidsUpload
from seemingly failing if they are aborted during thePutObjectTaggingCommand
phase, and the upload acutally succeeded (since nothing actually cancelled it).abortController
option was updated to not cause a typing issue when used with globalAbortController
(no longer need to@ts-ignore
).Testing
Not tested.
Additional context
We encountered upload "hangs" in our code, similar to those described in #6426 . While investigating, I encountered several opportunities for improvement, as noted in the Description above.
Checklist
*.integ.spec.ts
).@public
tag and enable doc generation on the package?By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.