Abort Download Object #80

waahm7 · 2024-12-04T18:23:54Z

Issue #, if available:
#65

Description of changes:
This PR implements the drop functionality for downloads. Here are the changes:

Remove the join function. It was working more like abort function since it closes the body. That behavior is not right because join function should wait for the download to complete instead of aborting it and then only waiting for the in-progress parts to complete. I have removed join function as it didn't made much sense, customers should consume the output if they want to wait for the download to complete. If we don't close the body in the join function, it may hang waiting for the body channel to clear.
Add abort function which cancels any in-progress work and any future work.
If any part fails with an error, cancel any in-progress work and future work.
Rename body->output, this makes it more generic because we are not just delivering body from it. We are delivering metadata and any error that occurs will be delivered to this channel. If there is an error, the channel will be closed and we will not get any more data from this channel. This consolidates our error reporting to a single place instead of join + body channel.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…nto cancel-single-download

aajtodd · 2024-12-05T15:18:33Z

aws-s3-transfer-manager/src/operation/download.rs

 /// Operation builders
 pub mod builders;
+/// Abstractions for responses and consuming data streams.
+pub mod output;


fix: Be consistent across operations. e.g. upload output module is not public but it re-exports the output type

Thanks, updated.

aajtodd · 2024-12-05T15:24:48Z

aws-s3-transfer-manager/src/operation/download/output.rs

 ///
 /// Wraps potentially multiple streams of binary data into a single coherent stream.
 /// The data on this stream is sequenced into the correct order.
 #[derive(Debug)]
-pub struct Body {
-    inner: UnorderedBody,
+pub struct Output {


suggestion/discuss: Should this be consistent with the other operations output types (e.g. DownloadOutput)?

Thanks, yeah, DownloadOutput makes more sense since there are different types of outputs. I have updated it.

aajtodd · 2024-12-05T15:26:49Z

aws-s3-transfer-manager/src/operation/download/output.rs

@@ -42,35 +42,35 @@ pub struct ChunkOutput {
 // TODO: Do we want to expose something to yield multiple chunks in a single call, like
 // recv_many/collect, etc.? We can benchmark to see if we get a significant performance boost once
 // we have a better scheduler in place.
-impl Body {
-    /// Create a new empty body
+impl Output {


future: We may want to have a conversion that allows you to just access the bytes off the stream (i.e. user only cares about the contents and not any of the intermediate metadata), we can wait though as it can be added in a backwards compatible way

fn into_body(self) -> Body

Thanks, currently, body vs. output is not very different; it's only .next() vs. .next().data. We can add this util in the future if we add more things to the output.

Discussed offline, reverted the name change.

aws-s3-transfer-manager/src/operation/download.rs

aajtodd · 2024-12-05T15:38:46Z

aws-s3-transfer-manager/tests/download_test.rs

+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_abort_download() {


You don't have to necessarily use it here but one thing to make you aware of is you can leverage capture_test_logs (example) which you can use to assert the logs contain certain things (e.g. to verify particular branches were hit).

ysaito1001

Looks good, leaving minor comments.

aws-s3-transfer-manager/examples/cp.rs

aws-s3-transfer-manager/src/operation/download/handle.rs

aws-s3-transfer-manager/src/operation/download_objects/worker.rs

graebm · 2024-12-06T00:39:23Z

aws-s3-transfer-manager/src/operation/download/output.rs

@@ -42,35 +42,35 @@ pub struct ChunkOutput {
 // TODO: Do we want to expose something to yield multiple chunks in a single call, like
 // recv_many/collect, etc.? We can benchmark to see if we get a significant performance boost once
 // we have a better scheduler in place.
-impl Body {
-    /// Create a new empty body
+impl DownloadOutput {


Inconsistent naming: We currently have UploadOutput UploadObjectsOutput and DownloadObjectsOutput, which are single structs representing the end result of an operation.

But this represents a sequence of structs.

Also, we'll eventually need something representing the end result of a download operation (where we'll put stuff like whether checksum validation happened). I would have expected that to be namedDownloadOutput.

If this continues to be a sequence of ChunkOutput, I'm fine just continuing to call this Body

If we were going to change it to be a sequence of enums (representing, Discovery, Chunk, Final, etc), then maybe I'd name it Events instead, but I don't think that's the plan

Thanks, discussed offline, I have reverted the naming to body.

aws-s3-transfer-manager/src/operation/download/service.rs

graebm · 2024-12-06T18:51:49Z

aws-s3-transfer-manager/src/operation/download/service.rs

+                    );
+                }
+            }
+
            if let Err(err) = comp_tx.send(resp).await {


Is it OK that ErrorKind::OperationCancelledgets sent oncomp_tx`? That could result in MANY chunks sending their cancellation error on the chunk transmitter

Do all of these end up reported to the user via the Body/Output iterator? Or does it filter errors so the user only sees the 1st? Can we guarantee that the ACTUAL error gets reported to the user before any cancellation errors?

As soon as we encounter an error, we close the channel so user will only see the first error.

Can we guarantee that the ACTUAL error gets reported to the user before any cancellation errors?

Tokio does guarantee that "All data sent on Sender will become available on Receiver in the same order as it was sent." However, if the channel is full, all of them will be waiting at an await point. In that case (not 100% sure), the order won't be guaranteed. We will need a fair bit of complexity to work around that, i.e., maybe collect all the errors and then only send those that are not OperationCancelled errors. Not sure if there is a simpler way to do this. @aajtodd any ideas?

Do we need to send operation cancelled across the channel at all?

I was thinking there can be a case where only the operation cancelled error is there, but yeah, we can just not send that error at all.

A user wouldn't act on that error right? I wouldn't think so anyway, it doesn't tell them anything they don't already know if they cancelled the operation, thus it probably doesn't make sense to yield it to them.

Thanks, I have updated it to ignore OperationCancelled errors.

Co-authored-by: Michael Graeb <[email protected]>

aws-s3-transfer-manager/src/operation/download/handle.rs

Co-authored-by: Michael Graeb <[email protected]>

waahm7 and others added 23 commits November 27, 2024 10:01

wip get rid of join and add abort function

2cd62a2

wip watch channel

14a9130

Support cancelling multiple downloads

daf50ec

Merge remote-tracking branch 'origin/ysaito/cancel-multi-downloads' i…

7956233

…nto cancel-single-download

wip handle discovery errors

0b9b305

rename -> body to output

8226092

fix cp

2b36ded

use common types

21b7d2d

update exisitng test

fd470f8

update drain

bf879f1

more body->output renames

a44575b

log error

31b6241

debug log

c570114

fmt

001ff19

fix todos

51493c2

fmt

d20ec39

get rid of todo

0839743

no need for box error

c8ffadb

Merge branch 'main' into cancel-single-download

c25ba12

debug

ebcc96d

add test

d954fbc

fmt

80e587e

fix todo

0462715

waahm7 changed the title ~~WIP | Abort Download Object~~ Abort Download Object Dec 4, 2024

waahm7 marked this pull request as ready for review December 4, 2024 19:46

waahm7 requested a review from a team as a code owner December 4, 2024 19:46

handle abort

05c2504

waahm7 mentioned this pull request Dec 4, 2024

Follow up on PR#75 #78

Merged

aajtodd reviewed Dec 5, 2024

View reviewed changes

rename output to DownloadOutput and pub use

4e55abd

change logs

227180a

ysaito1001 reviewed Dec 5, 2024

View reviewed changes

fix typo

f0e7d2b

ysaito1001 approved these changes Dec 5, 2024

View reviewed changes

graebm reviewed Dec 6, 2024

View reviewed changes

waahm7 and others added 8 commits December 6, 2024 11:08

Update aws-s3-transfer-manager/src/operation/download/service.rs

aba4be5

Co-authored-by: Michael Graeb <[email protected]>

fmt

a0da2b4

ignore OperationCancelled errors

79c660b

revert name to body

ece39ae

mising reverts

471d7bf

more

26f3d81

more

63a2158

fmt

93d58e8

aajtodd approved these changes Dec 9, 2024

View reviewed changes

graebm approved these changes Dec 9, 2024

View reviewed changes

aws-s3-transfer-manager/src/operation/download/handle.rs Outdated Show resolved Hide resolved

aws-s3-transfer-manager/src/operation/download/handle.rs Outdated Show resolved Hide resolved

waahm7 and others added 3 commits December 9, 2024 09:03

Update aws-s3-transfer-manager/src/operation/download/handle.rs

34f994f

Co-authored-by: Michael Graeb <[email protected]>

update doc

358c72d

more parts

1420ec7

waahm7 merged commit 540285a into main Dec 9, 2024
13 checks passed

waahm7 deleted the cancel-single-download branch December 9, 2024 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abort Download Object #80

Abort Download Object #80

waahm7 commented Dec 4, 2024 •

edited

Loading

aajtodd Dec 5, 2024

waahm7 Dec 5, 2024

aajtodd Dec 5, 2024

waahm7 Dec 5, 2024

aajtodd Dec 5, 2024

waahm7 Dec 5, 2024 •

edited

Loading

waahm7 Dec 6, 2024

aajtodd Dec 5, 2024

ysaito1001 left a comment

graebm Dec 6, 2024

graebm Dec 6, 2024

waahm7 Dec 6, 2024

graebm Dec 6, 2024

waahm7 Dec 6, 2024

aajtodd Dec 6, 2024

waahm7 Dec 6, 2024

aajtodd Dec 6, 2024

waahm7 Dec 6, 2024

Abort Download Object #80

Abort Download Object #80

Conversation

waahm7 commented Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waahm7 Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysaito1001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waahm7 commented Dec 4, 2024 •

edited

Loading

waahm7 Dec 5, 2024 •

edited

Loading