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

Allow CallbackExceptionHandler to be set on DeferredManager and Promise #135

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

aalmiray
Copy link
Contributor

@aalmiray aalmiray commented Oct 28, 2017

The result of a quick spike to figure out if it's possible to define CallbackExceptionHandler on both DeferredManager and Promise in order to have localized exception handling. The current rules are:

  • setting CEH on a Promise (once submitted) takes precedence over the other CEH (as long as it's set before any callback resolves).
  • every Promise passes on its CEH to any Promise created by it, such as FilteredPromise and PipedPromise, in other words children promises inherit the CEH from their parent.
  • setting CEH on DeferredManager sets it on every Promise created by this DM.

Precedence of invocation for CEH is thus set as: promise, dm, global.
Please have a look at FailureTest to see how this mechanism works.


This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+1.0%) to 55.717% when pulling 9c8e08e on aalmiray:local-exception-handler into 395e988 on jdeferred:2.x.

@saturnism
Copy link
Member

My first thought at this is, what if the promise is shared w/ multiple consumers? Each consumer can attach their done/fail callbacks. But when it comes to callback exception handling, they could overwrite each other.


Review status: 0 of 20 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@aalmiray
Copy link
Contributor Author

indeed. There are other problems such as: let's assume you have a Promise created by dm1, combine it with other promises from dm2 -> the first promise now has it's CEH set by dm2 (if non-null, I missed that check). This definitely requires more work, specially coming up with possible scenarios


Review status: 0 of 20 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants