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

Parsing logic: fail gracefully on unexpected input #1237

Merged
merged 17 commits into from
Jan 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -540,14 +540,12 @@ set(IO_SOURCE
src/IO/ADIOS/ADIOS2PreloadAttributes.cpp
src/IO/InvalidatableFile.cpp)
set(IO_ADIOS1_SEQUENTIAL_SOURCE
src/Error.cpp
src/auxiliary/Filesystem.cpp
src/ChunkInfo.cpp
src/IO/ADIOS/CommonADIOS1IOHandler.cpp
src/IO/ADIOS/ADIOS1IOHandler.cpp
src/IO/IOTask.cpp)
set(IO_ADIOS1_SOURCE
src/Error.cpp
src/auxiliary/Filesystem.cpp
src/ChunkInfo.cpp
src/IO/ADIOS/CommonADIOS1IOHandler.cpp
Expand Down Expand Up @@ -674,6 +672,9 @@ if(openPMD_HAVE_ADIOS1)
target_compile_definitions(openPMD.ADIOS1.Serial PRIVATE openPMD_HAVE_ADIOS1=1)
target_compile_definitions(openPMD.ADIOS1.Serial PRIVATE openPMD_HAVE_MPI=0)
target_compile_definitions(openPMD.ADIOS1.Serial PRIVATE _NOMPI) # ADIOS header
# This ensures that the ADIOS1 targets don't ever include Error.hpp
# To avoid incompatible error types in weird compile configurations
target_compile_definitions(openPMD.ADIOS1.Serial PRIVATE OPENPMD_ADIOS1_IMPLEMENTATION)

if(openPMD_HAVE_MPI)
set_target_properties(openPMD.ADIOS1.Parallel PROPERTIES
Expand Down Expand Up @@ -711,6 +712,9 @@ if(openPMD_HAVE_ADIOS1)
target_compile_definitions(openPMD.ADIOS1.Parallel PRIVATE openPMD_HAVE_MPI=0)
target_compile_definitions(openPMD.ADIOS1.Parallel PRIVATE _NOMPI) # ADIOS header
endif()
# This ensures that the ADIOS1 targets don't ever include Error.hpp
# To avoid incompatible error types in weird compile configurations
target_compile_definitions(openPMD.ADIOS1.Parallel PRIVATE OPENPMD_ADIOS1_IMPLEMENTATION)

# Runtime parameter and API status checks ("asserts")
if(openPMD_USE_VERIFY)
Expand Down Expand Up @@ -909,6 +913,7 @@ if(openPMD_USE_INVASIVE_TESTS)
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

#ifdef openPMD_USE_INVASIVE_TESTS

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I see.
ok, extents its meaning but is ok.

endif()

if(openPMD_BUILD_TESTING)
Expand Down
41 changes: 41 additions & 0 deletions include/openPMD/Error.hpp
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
#pragma once

#include "openPMD/ThrowError.hpp"

#include <exception>
#include <optional>
#include <string>
#include <utility>
#include <vector>

#if defined(OPENPMD_ADIOS1_IMPLEMENTATION)
static_assert(false, "ADIOS1 implementation must not include Error.hpp");
#endif

namespace openPMD
{
/**
Expand Down Expand Up @@ -80,5 +87,39 @@ namespace error
public:
Internal(std::string const &what);
};

/*
* Read error concerning a specific object.
*/
class ReadError : public Error
{
public:
AffectedObject affectedObject;
Reason reason;
// If empty, then the error is thrown by the frontend
std::optional<std::string> backend;
std::string description; // object path, further details, ...

ReadError(
AffectedObject,
Reason,
std::optional<std::string> backend_in,
std::string description_in);
};

/*
* Inrecoverable parse error from the frontend.
*/
class ParseError : public Error
{
public:
ParseError(std::string what);
};
} // namespace error

/**
* @brief Backward-compatibility alias for no_such_file_error.
*
*/
using no_such_file_error = error::ReadError;
} // namespace openPMD
7 changes: 5 additions & 2 deletions include/openPMD/IO/ADIOS/ADIOS2IOHandler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,9 @@ namespace detail
template <typename BA>
void enqueue(BA &&ba, decltype(m_buffer) &);

template <typename... Args>
void flush(Args &&...args);

struct ADIOS2FlushParams
{
/*
Expand Down Expand Up @@ -1103,7 +1106,7 @@ namespace detail
* deferred IO tasks had been queued.
*/
template <typename F>
void flush(
void flush_impl(
ADIOS2FlushParams flushParams,
F &&performPutsGets,
bool writeAttributes,
Expand All @@ -1114,7 +1117,7 @@ namespace detail
* and does not flush unconditionally.
*
*/
void flush(ADIOS2FlushParams, bool writeAttributes = false);
void flush_impl(ADIOS2FlushParams, bool writeAttributes = false);

/**
* @brief Begin or end an ADIOS step.
Expand Down
52 changes: 43 additions & 9 deletions include/openPMD/IO/AbstractIOHandler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,18 +34,10 @@
#include <queue>
#include <stdexcept>
#include <string>
#include <type_traits>

namespace openPMD
{
class no_such_file_error : public std::runtime_error
{
public:
no_such_file_error(std::string const &what_arg)
: std::runtime_error(what_arg)
{}
virtual ~no_such_file_error()
{}
};

class unsupported_data_error : public std::runtime_error
{
Expand Down Expand Up @@ -143,6 +135,48 @@ namespace internal
///< Special state only active while internal routines are
///< running.
};

// @todo put this somewhere else
template <typename Functor, typename... Args>
auto withRWAccess(SeriesStatus &status, Functor &&functor, Args &&...args)
-> decltype(std::forward<Functor>(functor)(std::forward<Args>(args)...))
{
using Res = decltype(std::forward<Functor>(functor)(
std::forward<Args>(args)...));
if constexpr (std::is_void_v<Res>)
{
auto oldStatus = status;
status = internal::SeriesStatus::Parsing;
try
{
std::forward<decltype(functor)>(functor)();
}
catch (...)
{
status = oldStatus;
throw;
}
status = oldStatus;
return;
}
else
{
auto oldStatus = status;
status = internal::SeriesStatus::Parsing;
Res res;
try
{
res = std::forward<decltype(functor)>(functor)();
}
catch (...)
{
status = oldStatus;
throw;
}
status = oldStatus;
return res;
}
}
} // namespace internal

/** Interface for communicating between logical and physically persistent data.
Expand Down
9 changes: 6 additions & 3 deletions include/openPMD/IO/AbstractIOHandlerImpl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -208,10 +208,13 @@ class AbstractIOHandlerImpl
{
std::cerr << "[AbstractIOHandlerImpl] IO Task "
<< internal::operationAsString(i.operation)
<< " failed with exception. Removing task"
<< " from IO queue and passing on the exception."
<< " failed with exception. Clearing IO queue and "
"passing on the exception."
<< std::endl;
(*m_handler).m_work.pop();
while (!m_handler->m_work.empty())
{
m_handler->m_work.pop();
}
throw;
}
(*m_handler).m_work.pop();
Expand Down
2 changes: 2 additions & 0 deletions include/openPMD/Iteration.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,8 @@ class Iteration : public Attributable
std::string filePath, std::string const &groupPath, bool beginStep);
void readGorVBased(std::string const &groupPath, bool beginStep);
void read_impl(std::string const &groupPath);
void readMeshes(std::string const &meshesPath);
void readParticles(std::string const &particlesPath);

/**
* Status after beginning an IO step. Currently includes:
Expand Down
14 changes: 13 additions & 1 deletion include/openPMD/ReadIterations.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,21 @@ class SeriesIterator

std::optional<SeriesIterator *> nextIterationInStep();

std::optional<SeriesIterator *> nextStep();
/*
* When a step cannot successfully be opened, the method nextStep() calls
* itself again recursively.
* (Recursion massively simplifies the logic here, and it only happens
* in case of error.)
* After successfully beginning a step, this methods needs to remember, how
* many broken steps have been skipped. In case the Series does not use
* the /data/snapshot attribute, this helps figuring out which iteration
* is now active. Hence, recursion_depth.
*/
std::optional<SeriesIterator *> nextStep(size_t recursion_depth);

std::optional<SeriesIterator *> loopBody();

void deactivateDeadIteration(iteration_index_t);
};

/**
Expand Down
8 changes: 7 additions & 1 deletion include/openPMD/Series.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -611,9 +611,15 @@ OPENPMD_private
* Iterations/Records/Record Components etc.
* If series.iterations contains the attribute `snapshot`, returns its
* value.
* If do_always_throw_errors is false, this method will try to handle errors
* and turn them into a warning (useful when parsing a Series, since parsing
* should succeed without issue).
* If true, the error will always be re-thrown (useful when using
* ReadIterations since those methods should be aware when the current step
* is broken).
*/
std::optional<std::deque<IterationIndex_t> >
readGorVBased(bool init = true);
readGorVBased(bool do_always_throw_errors, bool init);
void readBase();
std::string iterationFilename(IterationIndex_t i);

Expand Down
70 changes: 70 additions & 0 deletions include/openPMD/ThrowError.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/* Copyright 2022 Franz Poeschel
*
* This file is part of openPMD-api.
*
* openPMD-api is free software: you can redistribute it and/or modify
* it under the terms of of either the GNU General Public License or
* the GNU Lesser General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* openPMD-api is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License and the GNU Lesser General Public License
* for more details.
*
* You should have received a copy of the GNU General Public License
* and the GNU Lesser General Public License along with openPMD-api.
* If not, see <http://www.gnu.org/licenses/>.
*/

/*
* For objects that must not include Error.hpp but still need to throw errors.
* In some exotic compiler configurations (clang-6 with libc++),
* including Error.hpp into the ADIOS1 backend leads to incompatible error type
* symbols.
* So, use only the functions defined in here in the ADIOS1 backend.
* Definitions are in Error.cpp.
*/

#pragma once

#include "openPMD/auxiliary/Export.hpp"

#include <optional>
#include <string>
#include <vector>

namespace openPMD::error
{
enum class AffectedObject
{
Attribute,
Dataset,
File,
Group,
Other
};

enum class Reason
{
NotFound,
CannotRead,
UnexpectedContent,
Inaccessible,
Other
};

[[noreturn]] OPENPMDAPI_EXPORT void
throwBackendConfigSchema(std::vector<std::string> jsonPath, std::string what);

[[noreturn]] OPENPMDAPI_EXPORT void
throwOperationUnsupportedInBackend(std::string backend, std::string what);

[[noreturn]] OPENPMDAPI_EXPORT void throwReadError(
AffectedObject affectedObject,
Reason reason_in,
std::optional<std::string> backend,
std::string description_in);
} // namespace openPMD::error
2 changes: 0 additions & 2 deletions include/openPMD/auxiliary/JSON_internal.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@

#include "openPMD/config.hpp"

#include "openPMD/Error.hpp"

#include <nlohmann/json.hpp>
#include <toml.hpp>

Expand Down
1 change: 1 addition & 0 deletions include/openPMD/backend/Container.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ class Container : public Attributable
friend class Series;
template <typename>
friend class internal::EraseStaleEntries;
friend class SeriesIterator;

protected:
using ContainerData = internal::ContainerData<T, T_key, T_container>;
Expand Down
Loading