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

Make exception_to_status not abort #8009

Closed
wants to merge 1 commit into from
Closed

Conversation

jedelbo
Copy link
Contributor

@jedelbo jedelbo commented Aug 29, 2024

What, How & Why?

Probably better

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed
  • bindgen/spec.yml, if public C++ API changed

@cla-bot cla-bot bot added the cla: yes label Aug 29, 2024
@jedelbo jedelbo requested a review from jbreams August 29, 2024 10:57
Copy link

Pull Request Test Coverage Report for Build jorgen.edelbo_422

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • 79 unchanged lines in 18 files lost coverage.
  • Overall coverage decreased (-0.02%) to 91.096%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/realm/exceptions.cpp 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
src/realm/array_backlink.cpp 1 91.38%
src/realm/array_string.cpp 1 88.03%
src/realm/dictionary.cpp 1 85.16%
src/realm/sort_descriptor.cpp 1 94.06%
src/realm/util/file.cpp 1 84.84%
src/realm/util/serializer.cpp 1 90.43%
test/test_index_string.cpp 1 93.48%
src/realm/table_view.cpp 2 92.99%
src/realm/sync/noinst/protocol_codec.hpp 3 75.74%
src/realm/table.cpp 3 90.29%
Totals Coverage Status
Change from base Build 2589: -0.02%
Covered Lines: 217315
Relevant Lines: 238555

💛 - Coveralls

@jbreams
Copy link
Contributor

jbreams commented Aug 29, 2024

Having this condition being unreachable was a deliberate design choice so when totally unknown exception types like this happened we would at least get a stacktrace of where they were generated or rethrown. This has come up recently because NSError sometimes gets thrown from within the swift SDK. I think we should just add some support for converting NSError here or in the swift SDK rather than silently discarding all unknown exceptions. It seems like this change would make unknown exception types much harder to debug.

@nicola-cab
Copy link
Member

nicola-cab commented Aug 29, 2024

Probably worth setting the message string for the NSError that gets thrown. It seems a similar scenario we had to deal with for setting client reset callback errors happening in the SDK. We have had 2 crash reports so far ...

@jedelbo
Copy link
Contributor Author

jedelbo commented Aug 29, 2024

Probably worth setting the message string for the NSError that gets thrown.

The problem is that we cannot catch an NSError in our code. It should have been translated back to an Exception in the Swift SDK, I think.

@jedelbo
Copy link
Contributor Author

jedelbo commented Aug 29, 2024

We probably have to leave it as it is.

@jedelbo jedelbo closed this Aug 30, 2024
@jedelbo jedelbo deleted the je/exception-to-status branch August 30, 2024 07:37
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants