-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Adopt exception unification #8002
Conversation
9116625
to
35cfe06
Compare
if p.transferredBytes > 0 && p.isTransferComplete && !hasBeenFulfilled { | ||
ex.fulfill() | ||
hasBeenFulfilled = true | ||
DispatchQueue.main.async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a need to wrap this is a dispatch block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was a race condition at the end of the test? Without this we can return from the test function while the sync thread is still in the progress notification callback, and maybe that was an issue?
I clearly should have written a comment saying why because I don't remember and there isn't an obvious reason. Might just be incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!, Just some minor changes and some questions.
return translateSystemError(exception.code(), exception.what()); | ||
} | ||
|
||
__attribute__((objc_direct_members)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read that marking something as direct doesn't translate into a noticeable performance advantage, as it turns out that objc_msgSend
is very fast indeed. Is there any other reason for this or it is a performance and code size reduction decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just for the minor compiled size benefits.
|
||
/// A user info key containing the error code. This is provided for backwards | ||
/// compatiblity only and should not be used. | ||
extern NSString *const RLMErrorCodeKey __attribute((deprecated("use -[NSError code]"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add this to the CHANGELOG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was never actually documented anywhere afaict so it wasn't really part of the public API.
Realm/RLMRealm.mm
Outdated
*error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileWriteNoPermissionError | ||
userInfo:@{NSLocalizedDescriptionKey: @(e.what()), | ||
NSFilePathErrorKey: @(e.get_path().c_str())}]; | ||
// For backwards compatiblity, but this should go away in 11.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't if this been tried before, but we could have a label or tag to identify changes that need to occur in the next major release (11.0) which would make it so much easier to do clean up when we do this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dianaafanador3 Other teams create issues with a "NextMajor" label.
try { | ||
throw; | ||
} | ||
catch (realm::InvalidTransactionException const&) { | ||
@throw RLMException(@"Cannot modify Results outside of a write transaction."); | ||
catch (realm::WrongTransactionState const&) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why some of this exceptions were converted to LogicError in core an some left out to be handled by the SDKs like this one WrongTransactionState
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the exception messages from core are now fine to use as-is, and the two cases we're catching are where we want a different message. WrongTransactionState doesn't know what thing was being modified so it has a slightly less useful error message, and for OutOfBounds we're exactly mirroring NSArray's error message, mostly to simplify tests which check error messages on both managed and unmanaged collections.
if (_mutableSubscriptionSet != nil) { | ||
@throw RLMException(@"Cannot initiate a write transaction on subscription set that is already been updated."); | ||
if (_mutableSubscriptionSet) { | ||
@throw RLMException(@"Cannot initiate a write transaction on subscription set that is already being updated."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@throw RLMException(@"Cannot initiate a write transaction on subscription set that is already being updated."); | |
@throw RLMException(@"Cannot initiate a write transaction on a subscription set that is already being updated."); |
} | ||
_mutableSubscriptionSet = std::make_unique<realm::sync::MutableSubscriptionSet>(_subscriptionSet->make_mutable_copy()); | ||
realm::util::ScopeExit cleanup([&]() noexcept { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this is executed? trying to understand this deeper. Seems like this returns the mutableSubscriptionSet to null even after an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Whenever possible a function which throws an exception should leave an object in exactly the state it was in before the function call (in C++ terminology this is the "strong exception guarantee"). The refresh() means we don't quite hit this, but the important thing is that regardless of how we leave update:
, update:
should not leave the subscription set in a mid-update state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I forgot to answer the first half. The lambda is executed in the ScopeExit object's destructor, meaning that it's run when exiting the scope regardless of if it's via returning normally or due to an exception being thrown. It's the C++ equivalent of finally
or defer
and is a good place to put cleanup code that needs to be run on both success and error paths.
@@ -393,54 +393,6 @@ - (void)testCommitInOneNotificationDoesNotCancelOtherNotifications { | |||
[token2 invalidate]; | |||
} | |||
|
|||
#if 0 // Not obvious if there's still any way for notifiers to fail other than memory allocation failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the error handling should not make this test change instead of deleting it?, why we don't need to test this anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core removed the error reporting mechanism for notifiers since it's been dead code ever since core 6, so this test is now entirely obsolete.
335d975
to
c7e609b
Compare
c7e609b
to
0951fb4
Compare
b4893df
to
6d8d372
Compare
c91ca4e
to
bfa8cd9
Compare
bfa8cd9
to
def0d51
Compare
This isn't mergeable yet as it depends on realm/realm-core#5911 (and also a core release with exception unification merged), but I think the code changes here are complete and ready for review.
There's currently a few failing tests due to core bugs which have been fixed on master but not merged into the branch yet.