2.x: Make CompositeExcpetion thread-safe like 1.x and also fix some issues #4619
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ssues. Right now CompositeExcpetion has several issues: - `CompositeException(Throwable... exceptions)` doesn't deduplicate exceptions and flatten CompositeExceptions like `CompositeException(Iterable<? extends Throwable> errors)` - If using `CompositeException(Iterable<? extends Throwable> errors)` to create CompositeException, `suppress` cannot be used. - `suppress` doesn't update `cause`. - `suppress` doesn't deduplicate exceptions and flatten CompositeExceptions. - `suppress` and `Throwable.addSuppressed` are pretty confusing for Java 7+ users. Without looking at the implementation, it's hard to figure out the differences. This PR made the following changes: - Remove `CompositeException.suppress` so that it's easy to make CompositeException thread-safe. - This may cause some performance lost in some path rarely happening, e.g., an excpetion is thrown from `onError`, but that's not a big deal. - Since `suppress` is removed, it doesn't make sense to create an empty CompositeException, so `isEmpty` is removed and defense codes are added. - Defense codes for bad exceptions. - Deduplicate excepctions and flatten CompositeExceptions for `CompositeException(Throwable... exceptions)`.
Current coverage is 78.12% (diff: 68.75%)@@ 2.x #4619 diff @@
==========================================
Files 557 557
Lines 36296 36284 -12
Methods 0 0
Messages 0 0
Branches 5567 5566 -1
==========================================
- Hits 28359 28346 -13
+ Misses 5926 5925 -1
- Partials 2011 2013 +2
|
akarnokd
approved these changes
Sep 28, 2016
| @@ -266,15 +244,16 @@ public String getMessage() { | |||
| private List<Throwable> getListOfCauses(Throwable ex) { | |||
| List<Throwable> list = new ArrayList<Throwable>(); | |||
| Throwable root = ex.getCause(); | |||
| if (root == null) { | |||
| if (root == null || root == ex) { | |||
There was a problem hiding this comment.
Do you have a test for this? I removed it because code coverage showed it as never taken.
There was a problem hiding this comment.
| } | ||
| if (!ce.isEmpty()) { | ||
| if (!errors.isEmpty()) { | ||
| CompositeException ce = new CompositeException(errors); |
There was a problem hiding this comment.
Might be worth changing this so when there is only one exception, it doesn't wrap and lenghten the stacktrace unnecessarily.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now CompositeExcpetion has several issues:
CompositeException(Throwable... exceptions)doesn't deduplicate exceptions and flatten CompositeExceptions likeCompositeException(Iterable<? extends Throwable> errors)CompositeException(Iterable<? extends Throwable> errors)to create CompositeException,suppresscannot be used.suppressdoesn't updatecause.suppressdoesn't deduplicate exceptions and flatten CompositeExceptions.suppressandThrowable.addSuppressedare pretty confusing for Java 7+ users. Without looking at the implementation, it's hard to figure out the differences.This PR made the following changes:
CompositeException.suppressso that it's easy to make CompositeException thread-safe.onError, but that's not a big deal.suppressis removed, it doesn't make sense to create an empty CompositeException, soisEmptyis removed and defense codes are added.CompositeException(Throwable... exceptions).