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

Process Cancellation #20

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

jwelton
Copy link
Contributor

@jwelton jwelton commented Jun 9, 2016

Process Cancellation

The Problem

Currently, there is no way to cleanly exit out of long running processes such as zipping and unzipping files. There are many reasons for wanting to cancel these processes, so this pull request aims to provide a method for doing so.

The Solution

This solution uses a function call to perform a cancel operation on the current progress tracker, which is checked during the main loops for zipping and unzipping files. The function body (block) is registered after initialization of the current progress tracker. Once cancelled a ZipError.OperationCancelled error is thrown. This pull request also includes unit tests.

@RLovelett
Copy link

This is solving a problem that I too am interested in. 👍

It looks like this library already supports NSProgress. NSProgress already handles the ability to be canceled.

Is there any interest in combining these to capabilities?

@jwelton
Copy link
Contributor Author

jwelton commented Jun 9, 2016

@RLovelett That is a good point. NSProgress does have a flag already to support this. Commit 8e75ae3 replaces the flag with the property from the progress tracker.

@marmelroy
Copy link
Owner

Thanks a lot for this @jwelton (and @RLovelett for the input)! I think a lot of people would find this useful.

I have a small concern though... it's great that you are supporting NSProgress' cancellation ability but relying on a property of an optional progress tracker to determine whether or not the operation was cancelled doesn't feel like the best approach. I would prefer bringing back your original flag and allow NSProgress to set it from its cancellationHandler.

What do you reckon?

@jwelton
Copy link
Contributor Author

jwelton commented Jun 9, 2016

@marmelroy Yeah thats a fair point. Basing it on an optional is not ideal, however if we just set it within the completion handler of the progress tracker, then we are basically doing the same thing? It's still based on the optional, its just accessed through a required property.

Perhaps what we should do is just guard against the progress tracker being optional right after assignment? I know that it never will be, but at least this way the compiler knows that too?

Edit

I've made this change in ce3e13d. What do you think? It's not perfect, but it solves the optional problem.

@jwelton
Copy link
Contributor Author

jwelton commented Jun 10, 2016

I've been thinking about this and have decided to change my design slightly. I've now moved over to using a block as the cancellation trigger. This block is registered once the progress tracker has been created. I think this is a cleaner solution (certainly better than my last commit 😄 )

@jwelton
Copy link
Contributor Author

jwelton commented Jul 10, 2016

@marmelroy I don't mean to bother you, but I was wondering what you think of my latest solution for this problem?

@jwelton
Copy link
Contributor Author

jwelton commented Nov 25, 2016

@marmelroy I've just updated this branch with master (including Swift 3 changes). Are you happy with this solution?

@jengu
Copy link

jengu commented Oct 12, 2017

@marmelroy any updates on this? :(

@goa
Copy link

goa commented Jan 19, 2018

@marmelroy Any updates on this? I too am very much interested in this functionality. Thanks :)

@jwelton
Copy link
Contributor Author

jwelton commented Jan 19, 2018

@marmelroy I think actually we don't need to hold onto the progressTracker a the class level in this case. Tests are passing fine. Please review and let me know what you think.

@goa
Copy link

goa commented Jan 30, 2018

@jwelton Your solution works great and I like the fact that you also took the time to write a proper test, however in my use case of this library I need to be able to cancel only one unzip operation that is running concurrently with other unzip operations.

To handle such cases, I'm thinking that we could implement cancellation via a boolean value in the progress callback. If I have time I will try to implement it that way and let you know.

Alternatively we could return a unique id from all Zip and Unzip methods that could be used to cancel only one specific zip / unzip operation out of those running concurrently.

@jwelton
Copy link
Contributor Author

jwelton commented Jan 30, 2018

@goa Thanks for your suggestion. I agree it would be nice to have this cancellation behaviour per process rather than all or nothing. I think however implementing as part of the closure or via some ID is covering up the fact everything is static. If we didn't have it as static but instead instances, then we would be able to hold onto our instance and cancel on a per object basis.

However that would need to form part of a much bigger PR, which falls out of scope for what I was trying to achieve with this PR.

@AvdLee AvdLee requested a review from marmelroy June 15, 2018 08:15
@AvdLee
Copy link
Collaborator

AvdLee commented Jun 15, 2018

@marmelroy I still feel like your the best to review this PR. Can you give an update on @jwelton 's changes?

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.

6 participants