Throw on invalid file #3203
Throw on invalid file #3203
Conversation
src/realm/group.cpp
Outdated
| } | ||
| default: { | ||
| std::string err = | ||
| "Invalid top array size (ref, size): " + util::to_string(top_ref) + ", " + util::to_string(top_size); |
There was a problem hiding this comment.
| "Invalid top array size (ref, size): " + util::to_string(top_ref) + ", " + util::to_string(top_size); | |
| "Invalid top array size (ref=" + util::to_string(top_ref) + ", size=" + util::to_string(top_size) + "): "; |
|
What about the other asserts in open now that we are at it. If they assert anything about the content of the file, they should also throw instead of asserting. |
jedelbo
force-pushed
the
je/throw-on-invalid-file
branch
from
352cdd9
to
40f9008
Compare
jedelbo
force-pushed
the
je/throw-on-invalid-file
branch
from
40f9008
to
49b2e68
Compare
src/realm/group.cpp
Outdated
| REALM_ASSERT_RELEASE_EX(!"Invalid top array size", top_ref, top_size); | ||
| break; | ||
| } | ||
| validate_top_array(m_top, m_alloc); | ||
| m_table_names.init_from_parent(); | ||
| m_tables.init_from_parent(); | ||
| REALM_ASSERT_RELEASE_EX(m_table_names.size() == m_tables.size(), top_ref, m_table_names.size(), |
There was a problem hiding this comment.
Should this also throw instead of asserting?
CHANGELOG.md
Outdated
| @@ -1,7 +1,7 @@ | |||
| # NEXT RELEASE | |||
|
|
|||
| ### Enhancements | |||
| * None. | |||
| * An exception is thrown when a realm file has invalid top ref. | |||
There was a problem hiding this comment.
| * An exception is thrown when a realm file has invalid top ref. | |
| * Instead of asserting, an ??? exception is thrown when a realm file is opened with an invalid top ref. |
There was a problem hiding this comment.
A new commit with this change has been made
jedelbo
force-pushed
the
je/throw-on-invalid-file
branch
from
49b2e68
to
992c40c
Compare
jedelbo
force-pushed
the
je/throw-on-invalid-file
branch
from
992c40c
to
a53c8f3
Compare
| m_buffer += (std::string(" Path: ") + m_path); | ||
| return m_buffer.c_str(); | ||
| } | ||
|
|
There was a problem hiding this comment.
@jedelbo, are you aware that this construct makes the exception class not thread-compatible?
Modern C++ paradigm dictates these rules:
A necessary requirement for a type to be thread-compatible, is that all its const methods are thread-safe, meaning that two threads can safely call const methods on that type concurrently, as long as no other thread is calling a non-const method.
A class should be considered thread-compatible unless it is clear from the context that it is not, such as via documentation.
Roughly speaking, a type is thread-compatible if it is not thread-hostile. A type is thread-hostile if it does not offer the same intuitive level of thread-safety as is offered by fundamental types (integers) and almost all STL types (e.g., std::string).
There was a problem hiding this comment.
Yes, I was aware of that.
This will leave it up to the user to decide what to do