Process Cancellation

Open jwelton opened this issue 8 years ago • 13 comments

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.

Jun 09 '16 20:06 jwelton

This is solving a problem that I too am interested in. :+1:

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?

Jun 09 '16 20:06 RLovelett

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

Jun 09 '16 20:06 jwelton

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?

Jun 09 '16 21:06 marmelroy

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

Jun 09 '16 21:06 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 😄 )

Jun 10 '16 05:06 jwelton

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

Jul 10 '16 21:07 jwelton

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

Nov 25 '16 21:11 jwelton

@marmelroy any updates on this? :(

Oct 12 '17 12:10 jengu

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

Jan 19 '18 18:01 goa

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

Jan 19 '18 20:01 jwelton

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

Jan 30 '18 11:01 goa

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

Jan 30 '18 12:01 jwelton

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

Jun 15 '18 08:06 AvdLee

Zip Zip copied to clipboard