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

Canceling executor during uploadLargeFile() makes it hang. #16

Open
certainmagic opened this issue Oct 31, 2017 · 2 comments
Open

Canceling executor during uploadLargeFile() makes it hang. #16

certainmagic opened this issue Oct 31, 2017 · 2 comments
Assignees

Comments

@certainmagic
Copy link
Contributor

Marj did some great work and found a cool bug. :)

Currently, if you want to stop a large file upload, you need to call shutdown on the executor you provided. Unfortunately, if there are still tasks that haven't started uploading yet, calling shutdown returns them to the code that called shutdown and uploadLargeFile() ends up waiting in a call to get() on the corresponding future.

Thanks for tracking this down Marj!

@certainmagic certainmagic self-assigned this Oct 31, 2017
@certainmagic
Copy link
Contributor Author

Here's a stackoverflow question on this topic that Marj found:

https://stackoverflow.com/questions/27254869/future-get-hangs-when-callback-catches-interruptedexception

@certainmagic
Copy link
Contributor Author

We haven't figured out a way to get notified that the executor is shutdown.

We've discussed a few options...

(1) changing the future.get() calls to have a timeout and, if the timeout fires, check for the executor having been shutdown. this is basically polling. i has the benefit of not changing the API.

(2) providing a callback object that the uploader can use to ask "should i keep uploading?" callers to uploadLargeFile would be able to start returning false to cause a shutdown. this is another form of polling.

(3) making a new startLargeFileUpload() that returns a future, so that callers can use get() to wait for completion or call cancel() to ask for a clean cancelation of just that one upload (without having to stop the whole executor).

i think i prefer (3). while it changes the API, it's the cleanest and it's not polling. AND, the existing method can be reimplemented in terms of the new api. on the downside, we'd still have to tell people to not shutdown an executor until they're sure it's not in the middle of uploading.

as far as we know, this is a corner case and not actively hurting anyone, so we will think about it before rushing to make a fix.

thoughts?

malaysf added a commit that referenced this issue Feb 3, 2021
This behaves like `storeLargeFileFromLocalContent` but returns a cancellable future which will prevent additional tasks from starting and attempt to cancel in-progress API calls.

Addresses #16
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

No branches or pull requests

1 participant