Prevent observer from propagating exceptions

[MatthieuDartiailh]

Following phosphor logic, observers should be well behaved and never raise. If they do the exceptions are simply printed to stderr.

This avoid the old SystemError bug.

[codecov-io]

Codecov Report

Merging #71 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #71   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files          19       19           
  Lines         923      923           
=======================================
  Hits          910      910           
  Misses         13       13

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c063e52...e2c7266. Read the comment docs.

[MatthieuDartiailh]

Does that fit what you had in mind @sccolbert ? PyErr_Print never raises a new error, so on this side we are safe but it does not gives a full traceback, which may make debugging harder.

[MatthieuDartiailh]

ping @sccolbert

[MatthieuDartiailh]

We probably do not want to merge this before the next maintenance release planned mid-december. But this could use a review.

[frmdstryr]

I not a fan of this, I think it's better to have observers explicitly catch and handle exceptions (how it is now).

I think this will make it too easy to create unexpected / inconsistent model states.

[MatthieuDartiailh]

Actually this does lead to inconsistent states in enaml and I do agree with @sccolbert that an observer should not raise. Anyway I am not going to merge this as is. We discussed it with @sccolbert and we will provide a way to provide a custom handler for exception which can be used to simply re-raise the exception if it is what you need (while debugging for example).

[frmdstryr]

I re-read through the test you added and I can see how this change makes sense in that one failing observer shouldn't prevent other observers from being invoked as they may be be independent.

Django allows either case with it's signals framework via send and send_robust. Could something similar be done? Ex tag observe decorators and static observers with robust=True if this is made default allow errors to still be raised with onerror='raise' or something.

I tend to use observers a way of doing simple "transactions", ie set a member, if the observer raises an error, then revert the member back to a valid value, and raise the error to the UI (if needed) (see https://github.com/codelv/inkcut/blob/master/inkcut/job/view.enaml#L161). Perhaps this type of usage is unintended or improper :).

[MatthieuDartiailh]

I don't see a way to make the decision on the observer since that would require to order then to be sure to call all the robust ones first. However one thing I had in mind but that @sccolbert was considering overly complicated, was to keep track of all the exceptions that occurred and raise a meta exception summarizing them all at the end. I may try to implement it in atom but make it opt-in (the default behavior being the simple printing).

[Julian-O]

I am coming in late, but I would like to see this build on the Python logging library, so the output can be redirected to an appropriate file/email/syslog server, etc.

[MatthieuDartiailh]

I am not sure when I will come back to that, but I am less inclined to bare printing in particular because I fear it would break too much code. I am interested by people take on this issue. I feel like that offering the option to register a dedicated call back for handling exception and having it called in a differed manner could be a valid option. In particular I would like a way to prevent leaving an enaml app in a corrupted state because some observers were not called.