-
Notifications
You must be signed in to change notification settings - Fork 24
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
Comments
Here's a stackoverflow question on this topic that Marj found: |
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? |
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
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!
The text was updated successfully, but these errors were encountered: