Parsing logic: fail gracefully on unexpected input #1237
Merged
Conversation
This file contains hidden or 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
franzpoeschel
commented
Mar 25, 2022
| std::cerr << "WARNING: Skipping invalid openPMD record '" | ||
| << record_name << "'" << std::endl; | ||
| while (!IOHandler()->m_work.empty()) | ||
| IOHandler()->m_work.pop(); |
Contributor
Author
There was a problem hiding this comment.
No need to clear the queue here anymore, it's already cleared by AbstractIOHandler::flush() upon failure
franzpoeschel
force-pushed
the
topic-read-leniently
branch
2 times, most recently
from
5a5275f to
83dd2ad
Compare
franzpoeschel
commented
Mar 29, 2022
src/Iteration.cpp
Outdated
| std::make_optional<DeferredParseAccess>(std::move(dr)); | ||
| } | ||
|
|
||
| void Iteration::read() |
Contributor
Author
There was a problem hiding this comment.
Merged this with runDeferredParseAccess, no reason to keep them separate.
franzpoeschel
commented
Mar 29, 2022
| { | ||
| *newType = oldType; | ||
| throw; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
This style of try-catch-block is necessary if we want to keep the Series usable after throwing an exception. If we want to avoid this, we must find a way to parse without changing that state.
franzpoeschel
force-pushed
the
topic-read-leniently
branch
from
0795431 to
2ed1ba2
Compare
Contributor
Author
|
(not actually ready for review, just wanting to run the CI) |
franzpoeschel
force-pushed
the
topic-read-leniently
branch
2 times, most recently
from
8886071 to
1a6b89c
Compare
Contributor
|
This pull request introduces 1 alert when merging 1985a70 into 3c57afb - view on LGTM.com new alerts:
|
franzpoeschel
commented
Mar 31, 2022
1 task
franzpoeschel
force-pushed
the
topic-read-leniently
branch
from
4e2719e to
e4e00b3
Compare
Contributor
|
This pull request introduces 1 alert when merging e4e00b3 into 8b4a58a - view on LGTM.com new alerts:
|
Member
The most important pattern I have it reading a series (any encoding) from a sim that is currently running. The latest time step is usually incomplete. |
franzpoeschel
force-pushed
the
topic-read-leniently
branch
2 times, most recently
from
ec9defd to
91eed89
Compare
Contributor
|
This pull request introduces 1 alert when merging 91eed89 into 29bb474 - view on LGTM.com new alerts:
|
franzpoeschel
force-pushed
the
topic-read-leniently
branch
from
91eed89 to
0213774
Compare
Contributor
|
This pull request introduces 1 alert when merging 124b923 into 29bb474 - view on LGTM.com new alerts:
|
franzpoeschel
force-pushed
the
topic-read-leniently
branch
from
124b923 to
2397323
Compare
Contributor
|
This pull request introduces 1 alert when merging 47ec995 into 9c4173a - view on LGTM.com new alerts:
|
franzpoeschel
force-pushed
the
topic-read-leniently
branch
from
47ec995 to
20e1cf4
Compare
Contributor
|
This pull request introduces 1 alert when merging b64988f into 9c4173a - view on LGTM.com new alerts:
|
franzpoeschel
force-pushed
the
topic-read-leniently
branch
from
2117261 to
bf43dbe
Compare
1 task
franzpoeschel
force-pushed
the
topic-read-leniently
branch
from
bf43dbe to
6d08b14
Compare
Member
|
Thanks a lot! I did a first test reading partially written simulation data, i.e., the |
Contributor
Author
|
This is the output that I get upon Weirdly enough, the segfault only appears when redirecting stderr into a file.... update: we use |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ax3l
reviewed
Dec 20, 2022
Member
ax3l
left a comment
ax3l
left a comment
There was a problem hiding this comment.
This looks great!
Added inline comments :)
| message(WARNING "Invasive tests that redefine class signatures are " | ||
| "known to fail on Windows!") | ||
| endif() | ||
| target_compile_definitions(openPMD PRIVATE openPMD_USE_INVASIVE_TESTS=1) |
Member
There was a problem hiding this comment.
This macro should never be shipped. It introduces UB and is only used in some testing.
Suggested change
| target_compile_definitions(openPMD PRIVATE openPMD_USE_INVASIVE_TESTS=1) |
Contributor
Author
There was a problem hiding this comment.
The macro is still activated only in those special tests and not shipped (see the if(openPMD_USE_INVASIVE_TESTS) above). I need this macro inside Iteration.cpp in here:
Line 542 in 8c0d2a2
This is the most reliable way to trigger errors to be thrown.
Are there workflows where users compile openPMD with openPMD_USE_INVASIVE_TESTS and still install the resulting compiled library? If yes, are there better ways to do this then?
Just removing this line breaks the parsing test.
Member
There was a problem hiding this comment.
ah, I see.
ok, extents its meaning but is ok.
Some weird compiler configurations don't understand error type symbols across libraries, so keep the symbols entirely to the main library.
This is the commit with the most complicated logic changes. It implements skipping broken iterations at the Series level. This includes the Streaming API (Series::readIterations()), as well as our three iteration encodings. On the positive side: The most complex logic changes have already been prepared in the topic-adios2-append PR
Nothing too complicated, just tedious
`using no_such_file_error = error::ReadError` for backwards compatibility in user code.
@todo: Check if this needs to be done elsewhere
franzpoeschel
force-pushed
the
topic-read-leniently
branch
from
f74b697 to
5026e21
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Reading failures should ideally affect only the innermost scope possible. When one record component fails parsing due to whatever error, the neighboring record components should remain readable. When one iteration fails parsing, skip it and go on to the next.
Current behavior: Parsing fails and the Series cannot be opened
Close #983
TODO
ReadErrorerror type that backends can throw to tell the frontend about recoverable failuresReadErrorinside ADIOS1 and ADIOS2 attribute reading routinesReadErrors(partially implemented so far)Other backends,also consider dataset reading, ... UPDATE: Catching wrong attribute reads now possible in all backends, dataset reading and further things can come in a later updatefile-based, group-based, varialbe-based, deferred/eager, opening single iterations of a file-based series,streaming API, skipping iterationbp4_steps test seems to not write snapshot attributeonly written in new ADIOS2 schema as it should beThis whole PR is a bit of a stretch-goal as there are many different types and sources of errors to be recovered from.
@ax3l Can you name some specific error pattern (or even give me a test file) that the openPMD-api should handle more robustly?
I think that a backend failure should mean that the entire call to
AbstractIOHandler::flush()has failed and the queue is cleared. For finer granularity error handling, the frontend must issue singleflush()calls. So, I've changed error recovery procedures to clear the entire IO queue upon failure.