Skip to content
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

Shutdown task executor before aborting multipart uploads #1905

Merged
merged 1 commit into from
Apr 11, 2016

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented Apr 11, 2016

There's a race condition in which multipart uploads can still be
kept alive because aborting a multipart uploads can happen while
existing tasks are still being executed. We must shutdown the
executor before we can then abort multipart uploads.

This order was correct in the normal shutdown case, but for
the case where a user 'Ctrl-C's the process or where unexpected
exceptions propogate back to the S3 handler, the order was reversed.

In other words, the race condition exposed two possibilities:

Proper cleanup:

    t1 ---|start_part|-------------|end_part|---------------------
                                                                 |  join
    t2 ---|start_part|-------------|end_part|---------------------
                                                                 |  join
  main -----------------|ctrl-c|---------------|abort_upload|-----

Bad cleanup:

    t1 ---|start_part|--------------------------------|end_part|--
                                                                 |  join
    t2 ---|start_part|--------------------------------|end_part|--
                                                                 |  join
  main -----------------|ctrl-c|--|abort_upload|------------------

New behavior:

    t1 ---|start_part|---------------|end_part|-+
                                                | join
    t2 ---|start_part|---------------|end_part|-+
                                                | join
  main -----------------|ctrl-c|----------------+-------|abort_upload|

There's also a gap in test coverage here, so I've added a test case
for this case.

cc @kyleknap @JordonPhillips

There's a race condition in which multipart uploads can still be
kept alive because aborting a multipart uploads can happen while
existing tasks are still being executed.  We must shutdown the
executor before we can then abort multipart uploads.

This order was correct in the normal shutdown case, but for
the case where a user 'Ctrl-C's the process or where unexpected
exceptions propogate back to the S3 handler, the order was reversed.

In other words, the race condition exposed two possibilities:

Proper cleanup:

    t1 ---|start_part|-------------|end_part|---------------------
                                                                 |  join
    t2 ---|start_part|-------------|end_part|---------------------
                                                                 |  join
  main -----------------|ctrl-c|---------------|abort_upload|-----

Bad cleanup:

    t1 ---|start_part|--------------------------------|end_part|--
                                                                 |  join
    t2 ---|start_part|--------------------------------|end_part|--
                                                                 |  join
  main -----------------|ctrl-c|--|abort_upload|------------------

There's also a gap in test coverage here, so I've added a test case
for this case.
@kyleknap
Copy link
Contributor

🚢

@JordonPhillips
Copy link
Member

@jamesls jamesls merged commit c449942 into aws:develop Apr 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants