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

Reconsider catching exceptions #13

Closed
mbinna opened this issue May 3, 2014 · 16 comments · Fixed by #190
Closed

Reconsider catching exceptions #13

mbinna opened this issue May 3, 2014 · 16 comments · Fixed by #190

Comments

@mbinna
Copy link

mbinna commented May 3, 2014

Why does PromiseKit propagate exceptions (instances of the class NSException) to clients? Wouldn't it be more appropriate to follow the Cocoa convention of using errors (instances of the class NSError) exclusively to indicate failure?

Since clients rescue from erros instead of catch exceptions, I suggest to rename -[Promise catch] to -[Promise rescue] or -[Promise error]. If clients write code that throws exceptions, it is the clients fault that should be handled during development. Error handling at runtime should not occur by catching exceptions.

The Error Handling Programming Guide - A Note on Errors and Exceptions states the following:

It is important to keep in mind the difference between error objects and exception objects in Cocoa and Cocoa Touch, and when to use one or the other in your code. They serve different purposes and should not be confused.

Exceptions (represented by NSException objects) are for programming errors, such as an array index that is out of bounds or an invalid method argument. User-level errors (represented by NSError objects) are for runtime errors, such as when a file cannot be found or a string in a certain encoding cannot be read. Conditions giving rise to exceptions are due to programming errors; you should deal with these errors before you ship a product. Runtime errors can always occur, and you should communicate these (via NSError objects) to the user in as much detail as is required.

Although exceptions should ideally be taken care of before deployment, a shipped application can still experience exceptions as a result of some truly exceptional situation such as “out of memory” or “boot volume not available.” It is best to allow the highest level of the application—the global application object itself—to deal with these situations."

Let me know what you think.

@mxcl
Copy link
Owner

mxcl commented May 3, 2014

I agree and am aware of the convention that exceptions are “programmer errors” in Cocoa, but the convenience of being able to throw inside then handlers is large, and I have taught PromiseKit four times now, the first two the catch handler was named fail and by far the idea of Promises was much easier to grasp when I turned it around and made it seems like a try…catch block.

Also, I like the fault tolerant code I create now. Even out of bounds errors don't crash my program, and inside asynchronous blocks this is often desirable, instead of 1) tap button 2) crash the user gets 1) tap button 2) UIAlertView.

Finally, the error method is in fact supported in PromiseKit, if you return an NSError from any handler it is treated the same as an exception being thrown. The parameter to a catch block is always an NSError.

Edit: I guess throwing is not more convenient, but it is very explicit. I aim for code and libraries that you can show people and they “get it”.

@mbinna
Copy link
Author

mbinna commented May 4, 2014

Ah, I see. That makes sense. I was just curious about your intentions to go "all in" with both error handling and exception handling. Thank you for the explanation.

@mbinna mbinna closed this as completed May 4, 2014
@marklarr
Copy link

marklarr commented Mar 4, 2015

I like the idea of PromiseKit, but the fact that it catches all exceptions inside a promise and converts them to NSError is a deal-breaker for me. Catching exceptions in objective-c is a really bad idea. There should at least be an alternative solution for only catching NSErrors.

@mxcl
Copy link
Owner

mxcl commented Mar 4, 2015

There is: just define RETHROW_LIKE_A_MOFO.

https://github.com/mxcl/PromiseKit/blob/master/objc/PMKPromise.m#L173

However I can only say good things about catching all exceptions. You can still crash if you must, but it gives you the opportunity to do something first if you like. Just implement the PMKUnhandledErrorHandler block.

Personally I show a fatal error message and then crash, It's more user friendly.

@marklarr
Copy link

marklarr commented Mar 4, 2015

Do you also do that with the rest of your code? Catch any exception?

It's good to know that you can turn it off 👍 thanks for replying so quickly

@mxcl
Copy link
Owner

mxcl commented Mar 4, 2015

That's the only place it happens, it's not the policy of PromiseKit to catch any exceptions, it's just the policy of promise kit that any errors or exceptions that happen during asynchronous handling are caught so you have the opportunity to handle them. Otherwise you wouldn't even be able to catch the exception or handle the error even if you wanted to. Most other toolkits just swallow the error and return nil.

Review the code, it seems like you are worried about it and you shouldn't just take my word, or the word of anyone else who has put code on Github.

@marklarr
Copy link

marklarr commented Mar 4, 2015

Sorry, I wasn't clear. I was just curious if you try to catch exceptions in the rest of your applications and alert the user the way that you do in your asynchronous code.

@mxcl
Copy link
Owner

mxcl commented Mar 4, 2015

The way I use promises:

  1. chain everything that is a task so that I end up with a single promise for each logical separation of “task”
  2. if an error happens consider that task failed and restore UI to before the promise
  3. if it succeeds go to the next step

This way if an exception is thrown it is isolated to that promise chain and I can safely just show an error. My state machine is safe. Everything is cool.

I don't catch exceptions in the rest of the app, but inside the promise chain, it's fine.

I've enjoyed the additional stability promises provide. So have my clients.

@marklarr
Copy link

marklarr commented Mar 4, 2015

What if an exception in your promise chain dirties the state of your application? Dead Programs Tell No Lies and all that

@mxcl
Copy link
Owner

mxcl commented Mar 4, 2015

My promises are not coupled to the state of my application.

Cocoa exceptions are things like out of bounds accesses to arrays, or passing nil to an NSJSONSerialization.

I'm not sure I want my real life apps with 500,000 users to crash because some JSON provider returned invalid UTF8 data.

@marklarr
Copy link

marklarr commented Mar 4, 2015

To each their own, I suppose. Thanks for taking the time to explain everything.

@mxcl
Copy link
Owner

mxcl commented Mar 4, 2015

I can certainly see your perspective, and I've gone back on forth on it.

Maybe it is the wrong decision, but I think it benefits 95% of developers.

Perhaps it is no good for Venmo though, you guys are more mission critical than most.

Still, for users like yourselves I have provided mechanisms to have the promises work how you would like them to. Let me know if there is more than can be done.

@marklarr
Copy link

marklarr commented Mar 4, 2015

The preprocessor redefine is definitely good. Though, I no longer work on the Venmo iOS app, I now work on Ruby at Braintree for transaction processing. I was investigating this for a personal project :)

@mxcl
Copy link
Owner

mxcl commented Mar 5, 2015

I've been thinking about this all evening and maybe this should be reconsidered. If you consider exceptions programmer errors, and programmer errors that may lead to serious corruption, then they shouldn't be allowed to maintain the objects that they have any coupling.

I dunno. There are not many common exceptions that are thrown. Out of bounds array exceptions indicate a bad index, and this is potentially bad. Throwing because data is nil for NSJSONSerialization however is an example of something that should just be a runtime error. In that case we can catch that and make it into an error.

We could use some common exceptions in this thread to aid our decision.

@marklarr
Copy link

marklarr commented Mar 5, 2015

I think that it's up to the programmer to program defensively to avoid exceptions. In some languages (like Objective-C, unfortunately) this is harder than others. For example, in Ruby, indexing outside of an array just returns nil. While in Objective-C, you should check the array's length before accessing it to be safe.

Are these NSJSONSerialization and array exceptions coming from other frameworks you're using? They should hopefully just be checking if data is nil before parsing it for JSON, etc? Are they not?

@mxcl mxcl reopened this Mar 6, 2015
@mxcl mxcl changed the title "Fix" error handling Reconsider catching exceptions Mar 6, 2015
@mxcl
Copy link
Owner

mxcl commented May 2, 2015

PromiseKit 2 will not catch exceptions. After much deliberation I decided my choice for 1.x was wrong.

mxcl added a commit that referenced this issue May 12, 2015
Fixes #165
Fixes #56
Closes #18
Fixes #13
mxcl added a commit that referenced this issue May 12, 2015
Fixes #165
Fixes #56
Closes #18
Fixes #13
@mxcl mxcl mentioned this issue May 12, 2015
mxcl added a commit that referenced this issue May 12, 2015
Fixes #165
Fixes #56
Closes #18
Fixes #13
mxcl added a commit that referenced this issue May 13, 2015
Fixes #165
Fixes #56
Closes #18
Fixes #13
mxcl added a commit that referenced this issue May 13, 2015
Fixes #165
Fixes #56
Closes #18
Fixes #13
mxcl added a commit that referenced this issue May 13, 2015
Fixes #165
Fixes #56
Closes #18
Fixes #13
mxcl added a commit that referenced this issue May 13, 2015
Fixes #165
Fixes #56
Closes #18
Fixes #13
mxcl added a commit that referenced this issue May 13, 2015
Fixes #165
Fixes #56
Closes #18
Fixes #13
mxcl added a commit that referenced this issue May 14, 2015
Fixes #165
Fixes #56
Closes #18
Fixes #13
@mxcl mxcl closed this as completed in #190 May 14, 2015
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 a pull request may close this issue.

3 participants