Process Cancellation

[jwelton]

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]

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]

@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]

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]

@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]

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]

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

[jwelton]

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

[jengu]

@marmelroy any updates on this? :(

[goa]

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

[jwelton]

@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]

@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]

@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]

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