Skip to content

Handle exceptions fix #588

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 20 commits into from
Sep 25, 2018
Merged

Handle exceptions fix #588

merged 20 commits into from
Sep 25, 2018

Conversation

elizarov
Copy link
Contributor

  • Removed legacy onFinishing handler support from JobSupport.
      - It is no longer needed, because handleJobException covers Uncaught exception may not appear on console when main is waiting for a job #208
  • Fixed bugs that were masked by cancelling parent from onFinishing.
  • Consistent "Finishing" state was introduced in internal machinery:
      - Job enters finishing state when it is failing or it is completing
        and has children
      - Finishing state cleanly aggregates all failures, tracks cancellation
        and completion status
  • Job.isFailed is introduced as a consistent way to query the "failing"
      state of the job that was previously only implicitly available via
      invokeOnCompletion handler (cause != null means a failed job) and
      the documentation for both Job & Deferred is updated to reflect that.
  • Source-incompatible change: Job.invokeOnCompletion boolean parameter is
      change to onFailing. Such handlers are now invoked as soon as the job
      starts failing and the root cause exception of the failure is consistently
      passed to all the handlers.
  • The following internal methods were introduced to facilitate this:
      - Job.failParent(exception) is used by child to signal failure to parent
      - Job.cancelChild(parentJob) is used by parent to cancel child.
  • Child never aggregates exception received from it parent, but uses
      it as it root cause if there is no other exception.
  • JobSupport.handleJobException() logic for launch/actor is split into:
      - failParent - can be invoked multiple times on race;
      - handleJobException which is invoked exactly once.
  • Exception materiazization is much lazier now, which should
      significantly improve performance when cancelling large hierarchies.
  • Other minor perf improvements in JobSupport code.

Fixes #585

@qwwdfsad qwwdfsad self-requested a review September 20, 2018 12:19
@elizarov elizarov force-pushed the handle-exception-fix branch 2 times, most recently from 1bbd6bb to 1bb1b49 Compare September 22, 2018 19:17
@elizarov
Fix exception aggregation to ensure atomic handling of exceptions
ecbc85c 
* Removed legacy onFinishing handler support from JobSupport.
  - It is no longer needed, because handleJobException covers #208
* Fixed bugs that were masked by cancelling parent from onFinishing.
* Consistent "Finishing" state was introduced in internal machinery:
  - Job enters finishing state when it is failing or it is completing
    and has children
  - Finishing state cleanly aggregates all failures, tracks cancellation
    and completion status
* Job.isFailed is introduced as a consistent way to query the "failing"
  state of the job that was previously only implicitly available via
  invokeOnCompletion handler (cause != null means a failed job) and
  the documentation for both Job & Deferred is updated to reflect that.
* Source-incompatible change: Job.invokeOnCompletion boolean parameter is
  change to onFailing. Such handlers are now invoked as soon as the job
  starts failing and the root cause exception of the failure is consistently
  passed to all the handlers.
* The following internal methods were introduced to facilitate this:
  - Job.failParent(exception) is used by child to signal failure to parent
  - Job.cancelChild(parentJob) is used by parent to cancel child.
* Child never aggregates exception received from it parent, but uses
  it as it root cause if there is no other exception.
* JobSupport.handleJobException() logic for launch/actor is split into:
  - failParent - can be invoked multiple times on race;
  - handleJobException which is invoked exactly once.
* Exception materiazization is much lazier now, which should
  significantly improve performance when cancelling large hierarchies.
* Other minor perf improvements in JobSupport code.

Fixes #585
@elizarov
Fixed race between creation of a child & cancellation of parent job
7e23875
@elizarov
Minor optimization in fast-path of JobSupport.tryMakeCompleting
6634ed7 
Check for simple states on completing before looking for a first child
in a potentially long list of job nodes
@elizarov
Document JobSupport state-transition & notification time-line
ff0aab8
@elizarov
Introduced ChildHandle for attachChild
5d18d02 
* ChildHandle.childFailed controls notification from child to parent
  (no need to lookup parent job in the context)
* JobSupport.failsParent boolean property controls whether a job
  fails its parent on failure.
@elizarov elizarov force-pushed the handle-exception-fix branch from 1bb1b49 to 5d18d02 Compare September 23, 2018 16:11
elizarov and others added 2 commits September 23, 2018 19:14
@elizarov
Removed "cancel parent" from TestBase exception handlers
33e7ce4
@qwwdfsad
Job machinery improvements:
f157cec 
 * Code style & documentation
 * Use identity hashset for exception aggregation
qwwdfsad
qwwdfsad requested changes Sep 24, 2018
View reviewed changes
@qwwdfsad
Handle exception thrown by user-defined exception handler via global …
409ed26 
…exception handler, so such exception can't break coroutines machinery

Fixes #562
@qwwdfsad
Copy link
Member

qwwdfsad commented Sep 24, 2018 •
edited
Loading

Also, it's worth replacing JobCancellationException with CancellationException in unwrapping logic

@elizarov elizarov force-pushed the handle-exception-fix branch from 7171128 to ad84795 Compare September 24, 2018 20:48
@elizarov
Copy link
Contributor Author

Also, it's worth replacing JobCancellationException with CancellationException in unwrapping logic

Nope. That does not work. TimeoutException extends CancellationException and if we unwrap it, then it starts to break different things.

@elizarov
Child fails on parent failure (not cancelled)
f7a6334 
* This ensures transparency with respect to the fact that a job is
  a child in a hierarchy. When it fails, there is no side-effect of
  its being cancelled by its parent anymore.
* ChildJob and ChildHandle are made internal to further deter 3rd-party
  implementation and usage of parent-child machinery.
@elizarov elizarov force-pushed the handle-exception-fix branch from b4f0120 to f7a6334 Compare September 24, 2018 21:41
elizarov and others added 5 commits September 25, 2018 14:26
@elizarov
Abolish distinction between cancelled and failed Job/Deferred
6685fd0 
* Job.isCancelled is consistently true on any failure
* Deferred.isCompletedExceptionally is deprecated.
  It is equal to isCancelled && isCompleted
* CompletableDeferred.completeExceptionally is deprecated.
  Use cancel instead.

Fixes #220
@elizarov
Merge branch 'develop' into handle-exception-fix
1e0a2f0
@qwwdfsad
Fix lost exception during cancellation in produce, fix integration wi…
1b22af7 
…th play services after rebase
@elizarov
Reviewed and cleanup usage of "fail" word in comments and method names
852fae5
@elizarov
Job unwraps all instances of CancellationException
541a9b6 
* This makes exception handling fully consistent, since CancellationException
  is considered "normal", never goes to logs and does not cause
  parent cancellation.
* Solves exception transparency of withTimeout/withTimeoutOrNull --
  exception throws from inside gets rethrown outside
* Makes sure that closed/cancelled producer that throws
  ClosedSendChannelException is not considered to be failed
@qwwdfsad qwwdfsad merged commit 25b4388 into develop Sep 25, 2018
@qwwdfsad qwwdfsad deleted the handle-exception-fix branch September 26, 2018 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qwwdfsad qwwdfsad qwwdfsad requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elizarov @qwwdfsad @antanas-arvasevicius