Skip to content

GitLab

Commit 5b569948 authored by Paal Kvamme's avatar Paal Kvamme
Browse files

Improved error messages. SDAPI exceptions are now mostly wrapped in a ZgyError... 

Improved error messages. SDAPI exceptions are now mostly wrapped in a ZgyError exception, allowing OpenZGY to substitute a more readable message in the (regrettably few) cases it can do so.
parent 6ba0639c
Hide whitespace changes
Inline Side-by-side
native/src/api.cpp
View file @ 5b569948
......@@ -705,8 +705,9 @@ public:
_accessor.reset();
}
if (_fd) {
_fd->xx_close();
auto victim = _fd;
_fd.reset();
victim->xx_close();
}
// Metadata remains accessible. Not sure whether this is a good idea.
}
......
native/src/impl/file_sd.cpp
View file @ 5b569948
......@@ -56,6 +56,17 @@
#include <SDAPI/HttpContext.h>
#endif
// SDAPI footprint, might not be accurate.
// Use e.g. to look for places where a try/catch might be needed.
//
// SDGenericDataset constructors, destructor
// \b(open|close|readBlock|writeBlock|deleteBlock|getBlockNum|getBlocksSize|getSize|getConsistencyID|getSerializedContext)\(
// Not used yet in C++, the first two supported in Python+SdGlue.
// \b((get|set)(MetaData|SeismicMeta|Tags|ReadOnlyMode))\(
//
// SDManager constructors, destructor
// \b(setAuthProviderFrom(String|File|ImpToken)|setLogStatus)\(
/** \cond SSTORE */
/**
......@@ -376,74 +387,68 @@ DatasetInformation::DatasetInformation()
{
}
/**
* Create and poplulate an instance.
* Catching and translating exceptions from SDAPI is done by caller.
*/
DatasetInformation::DatasetInformation(seismicdrive::SDGenericDataset* sdgd)
: block_count_(0)
, block0_size_(0)
, block1_size_(0)
, last_block_size_(0)
{
try {
// Note that sdapi is a bit confusing with respect to signed/unsigned.
// getBlockNum() returns an unsigned (uint64_t).
// getSize() and getBlockSize() return long long, may return -1 as error.
// I will treat all of them as signed, since there is no way that
// there can be more than 2^63 blocks.
long long nblocks = sdgd->getBlockNum();
long long nbytes = sdgd->getSize();
std::vector<std::string> names;
if (nblocks >= 1)
names.push_back("0");
if (nblocks >= 2)
names.push_back("1");
if (nblocks >= 3)
names.push_back(std::to_string(nblocks - 1));
std::vector<long long> sizearray;
if (nblocks <= 0) {
}
#if 0 // Chicken...
else if (nblocks == 1) {
// Not using getSize() because I do not trust it.
// SDAPI requires a hint check_and_overwrite=true when writing
// a block that might exist already. If the hint is missing
// then getSize() will return the wrong result. OpenZGY has
// no control over who wrote the file. Defensive programming
// says to just don't use the getSize() until the current
// behavior (which I consider a bug) is fixed. Suggestion:
// would it be possible to scan the file on exit and fix up
// any incorrect size?
sizearray.push_back(nbytes);
}
#endif
else {
sizearray = sdgd->getBlocksSize(names);
}
if (nblocks < 0)
throw OpenZGY::Errors::ZgyInternalError("Unable to get block count for SDGenericDataset");
for (const auto size : sizearray)
if (size <= 0)
throw OpenZGY::Errors::ZgyInternalError("Unable to get segment size for SDGenericDataset");
this->block_count_ = nblocks;
this->block0_size_ = nblocks >= 1 ? sizearray.at(0) : 0;
this->block1_size_ = nblocks >= 2 ? sizearray.at(1) : 0;
this->last_block_size_ = nblocks >= 3 ? sizearray.at(2) : this->block1_size_;
// Not throwing exceptions here, because sdgd->getSize() is not reliable.
// But the problem *could* be that the block sizes of all the middle blocks
// is not the same. Which would be a real problem.
if (nbytes >= 0 && nbytes != (long long)totalSize())
SeismicStoreFile::_logger(0, "Dataset has inconsistent size");
SeismicStoreFile::_logger(1, toString());
// Note that sdapi is a bit confusing with respect to signed/unsigned.
// getBlockNum() returns an unsigned (uint64_t).
// getSize() and getBlockSize() return long long, may return -1 as error.
// I will treat all of them as signed, since there is no way that
// there can be more than 2^63 blocks.
long long nblocks = sdgd->getBlockNum();
long long nbytes = sdgd->getSize();
std::vector<std::string> names;
if (nblocks >= 1)
names.push_back("0");
if (nblocks >= 2)
names.push_back("1");
if (nblocks >= 3)
names.push_back(std::to_string(nblocks - 1));
std::vector<long long> sizearray;
if (nblocks <= 0) {
}
catch (const OpenZGY::Errors::ZgyError& ex) {
SeismicStoreFile::_logger(1, "Oops (DataSetInformation): " + std::string(ex.what()));
throw;
}
catch (const std::exception& ex) {
SeismicStoreFile::_logger(1, "Oops (DataSetInformation): " + std::string(ex.what()));
throw OpenZGY::Errors::ZgyInternalError("Seismic Store says: " + std::string(ex.what()));
#if 0 // Chicken...
else if (nblocks == 1) {
// Not using getSize() because I do not trust it.
// SDAPI requires a hint check_and_overwrite=true when writing
// a block that might exist already. If the hint is missing
// then getSize() will return the wrong result. OpenZGY has
// no control over who wrote the file. Defensive programming
// says to just don't use the getSize() until the current
// behavior (which I consider a bug) is fixed. Suggestion:
// would it be possible to scan the file on exit and fix up
// any incorrect size?
sizearray.push_back(nbytes);
}
#endif
else {
sizearray = sdgd->getBlocksSize(names);
}
if (nblocks < 0)
throw OpenZGY::Errors::ZgyInternalError("Unable to get block count for SDGenericDataset");
for (const auto size : sizearray)
if (size <= 0)
throw OpenZGY::Errors::ZgyInternalError("Unable to get segment size for SDGenericDataset");
this->block_count_ = nblocks;
this->block0_size_ = nblocks >= 1 ? sizearray.at(0) : 0;
this->block1_size_ = nblocks >= 2 ? sizearray.at(1) : 0;
this->last_block_size_ = nblocks >= 3 ? sizearray.at(2) : this->block1_size_;
// Not throwing exceptions here, because sdgd->getSize() is not reliable.
// But the problem *could* be that the block sizes of all the middle blocks
// is not the same. Which would be a real problem.
if (nbytes >= 0 && nbytes != (long long)totalSize())
SeismicStoreFile::_logger(0, "Dataset has inconsistent size");
SeismicStoreFile::_logger(1, toString());
}
std::string
......@@ -716,6 +721,8 @@ class SDGenericDatasetWrapper
std::string saved_tokentype_;
OpenZGY::SeismicStoreIOContext::tokencb_t tokenrefresh_;
std::string tokenrefreshtype_;
std::string tokenmessage_;
public:
typedef std::shared_ptr<SDGenericDatasetWrapper> Ptr;
SDGenericDatasetWrapper(std::shared_ptr<seismicdrive::SDManager> manager,
......@@ -725,6 +732,7 @@ public:
, mutex_()
, saved_token_(), saved_tokentype_()
, tokenrefresh_(), tokenrefreshtype_()
, tokenmessage_("Token not initialzed")
{
}
~SDGenericDatasetWrapper();
......@@ -744,6 +752,7 @@ public:
std::shared_ptr<const DatasetInformation> info() {
std::lock_guard<std::mutex> lk(mutex_);
if (!info_) {
try {
switch (disposition_) {
case OpenMode::Truncate:
info_.reset(new DatasetInformation());
......@@ -756,6 +765,10 @@ public:
default:
throw OpenZGY::Errors::ZgyInternalError("DatasetInformation: Dataset not open.");
}
}
catch (const std::exception& ex) {
throwCloudException(ex, "Initialize");
}
}
return info_;
}
......@@ -821,6 +834,36 @@ public:
return old != virgin_;
}
/**
* Called from the catch block after calling some method in SDAPI.
* If no token was provided, this is an "I told you so" error.
* It could have been reported earlier but it is possible that
* the cloud library somehow managed to handle authenticcation
* itself. The actual exception reported by SDAPI is in that case
* less interesting.
*
* A missing token is probably a user error (with "user" in this
* case being the application) and is reported as such.
*
* If the exception has alredy been wrapped by a ZgyException then
* it will be re-thrown without modification.
*/
void throwCloudException(const std::exception& ex, const char *message) {
SeismicStoreFile::_logger(1, std::stringstream()
<< "Oops (" << message << ")"
<< " (" << tokenmessage_ << "): "
<< ex.what());
if (dynamic_cast<const OpenZGY::Errors::ZgyError*>(&ex))
throw;
else if (!tokenmessage_.empty())
throw OpenZGY::Errors::ZgyUserError(tokenmessage_);
else
throw OpenZGY::Errors::ZgyInternalError
(std::string(message) + ": Seismic Store: " + std::string(ex.what()));
// TODO-Low: Should I just re-throw the sdapi error instead?
// TODO-Low: Either way, be more consistent about the answer.
}
/**
* Pass credentials down to the SDAPI layer.
*
......@@ -882,6 +925,17 @@ public:
std::string newtoken = tokencb ? tokencb() : token;
std::string newtokentype = tokencb ? tokencbtype : tokentype;
// A missing token at this point is probably an error, but in case
// the SDAPI library has some tricks up its sleeve don't throw
// am error unless the SDAPI does so first.
if (newtoken.empty())
if (tokencb)
tokenmessage_ = "Missing access token, first callback returned empty";
else
tokenmessage_ = "Missing access token or callback in iocontext";
else
tokenmessage_ = "";
authorizeManagerInSD(manager_.get(), newtoken, newtokentype);
// Save what we just set so reAuthorizeManager() won't need to set
......@@ -924,13 +978,20 @@ public:
newtokentype = tokenrefreshtype;
lk.lock();
if (saved_token_ != newtoken || saved_tokentype_ != newtokentype) {
if (newtoken.empty())
tokenmessage_ = "Missing access token, callback returned empty";
else
tokenmessage_ = "";
// In case of exception, always allow trying again.
saved_token_ = std::string();
saved_tokentype_ = std::string();
authorizeManagerInSD(manager_.get(), newtoken, newtokentype);
saved_token_ = newtoken;
saved_tokentype_ = newtokentype;
SeismicStoreFile::_logger(1, "A new token was provided");
if (newtoken.empty())
SeismicStoreFile::_logger(1, "The token was cleared");
else
SeismicStoreFile::_logger(1, "A new token was provided");
}
}
}
......@@ -959,11 +1020,12 @@ public:
}
dataset_->setExponentialRetryBackoffPolicy(&policy);
if (SeismicStoreFile::_logger(2, ""))
SeismicStoreFile::_logger(2, std::stringstream() << std::boolalpha
<< "Backoff enabled " << policy.enabled
<< " retries " << policy.maxRetry
<< " start " << (float)policy.initialWaitingTimeMicroSec*1.0e-6
<< " max " << (float)policy.maxWaitingTimeMicroSec*1.0e-6);
SeismicStoreFile::_logger
(2, std::stringstream()
<< "Backoff " << (policy.enabled ? "enabled" : "disabled")
<< " retries " << policy.maxRetry
<< " start " << (float)policy.initialWaitingTimeMicroSec*1.0e-6
<< " max " << (float)policy.maxWaitingTimeMicroSec*1.0e-6);
}
}
};
......@@ -1061,6 +1123,8 @@ SeismicStoreFile::SeismicStoreFile(const std::string& filename, OpenMode mode, c
context->_sdtokencb, context->_sdtoken_cbtype);
try {
if (mode != OpenMode::Closed)
datasetwrapper->setBackoff();
switch (mode) {
case OpenMode::Closed:
break;
......@@ -1074,17 +1138,9 @@ SeismicStoreFile::SeismicStoreFile(const std::string& filename, OpenMode mode, c
dataset->open(seismicdrive::SDDatasetDisposition::OVERWRITE, extra);
break;
}
if (mode != OpenMode::Closed)
datasetwrapper->setBackoff();
}
catch (const std::exception& ex) {
SeismicStoreFile::_logger(1, std::stringstream()
<< "Oops (Opening file mode "
<< int(mode) << "): "
<< ex.what());
throw OpenZGY::Errors::ZgyInternalError("Seismic Store says: " + std::string(ex.what()));
// TODO-Low: Should I just re-throw the sdapi error?
// TODO-Low: If not, should other SDAPI errors be translated?
datasetwrapper->throwCloudException(ex, "Open");
}
this->_dataset = datasetwrapper;
......@@ -1578,17 +1634,37 @@ SeismicStoreFile::xx_close()
}
if (_dataset) {
switch (_dataset->disposition()) {
case OpenMode::Closed:
break;
case OpenMode::ReadOnly:
case OpenMode::ReadWrite:
case OpenMode::Truncate:
if (_dataset->dataset()) {
this->_dataset->reAuthorizeManager();
_dataset->close();
try {
switch (_dataset->disposition()) {
case OpenMode::Closed:
break;
case OpenMode::ReadOnly:
case OpenMode::ReadWrite:
case OpenMode::Truncate:
if (_dataset->dataset()) {
auto victim = _dataset;
_dataset.reset();
try {
victim->reAuthorizeManager();
}
catch (const std::exception& ex) {
// Maybe we didn't need credentials just to close.
// So just log and swallow this one.
if (_logger(0, ""))
_logger(0, std::stringstream()
<< "Ignoring exception during close: "
<< ex.what());
}
victim->close();
}
break;
}
break;
}
catch (const std::exception& ex) {
// Too late; _dataset is null so we don't know the tokenmessage.
//_dataset->throwCloudException(ex, "Close");
throw OpenZGY::Errors::ZgyInternalError
("Close: Seismic Store: " + std::string(ex.what()));
}
}
_dataset.reset();
......@@ -1661,11 +1737,10 @@ SeismicStoreFile::deleteFile(const std::string& filename, bool missing_ok) const
<< "Deleting already deleted"
<< " \"" << filename << "\"\n");
if (!missing_ok)
throw;
_dataset->throwCloudException(ex, "Delete");
}
else {
// TODO-Low: Wrap in my own exception type or not?
throw;
_dataset->throwCloudException(ex, "Delete");
}
}
_logger(2, "SeismicStoreFile::deleteFile DONE.\n");
......@@ -1678,31 +1753,37 @@ SeismicStoreFile::altUrl(const std::string& filename) const
// to create an alturl from another alturl. Probably doesn't
// matter much either way.
// Make sure the returned smart pointer doesn't go out of scope.
std::shared_ptr<seismicdrive::SDManager> smart_manager = _dataset->manager();
seismicdrive::SDGenericDataset dataset(smart_manager.get(), filename);
dataset.open(seismicdrive::SDDatasetDisposition::READ_ONLY);
const std::string wid = dataset.getConsistencyID();
const std::string ctx = dataset.getSerializedContext();
const std::string url = filename.substr(0, filename.find("?")) + "?context=" + ctx;
try {
dataset.close();
// Make sure the returned smart pointer doesn't go out of scope.
std::shared_ptr<seismicdrive::SDManager> smart_manager = _dataset->manager();
seismicdrive::SDGenericDataset dataset(smart_manager.get(), filename);
dataset.open(seismicdrive::SDDatasetDisposition::READ_ONLY);
const std::string wid = dataset.getConsistencyID();
const std::string ctx = dataset.getSerializedContext();
const std::string url = filename.substr(0, filename.find("?")) + "?context=" + ctx;
try {
dataset.close();
}
catch (const std::exception& ex) {
// Workaround for what is probably a bug in SDAPI.
// Hopefully the dataset actually got closed anyway,
// so we don't get a resource leak here.
if (!strstr(ex.what(), "has been locked with different ID"))
throw;
_logger(0, "getSerializedContext() caused bogus exception for wid "+ wid + ": " + std::string(ex.what()));
}
_logger(2, std::stringstream()
<< "SeismicStoreFile::altUrl(\""
<< (filename.size() < 76 ? filename : filename.substr(0, 72) + "...")
<< "\")"
<< " = \"" << "REDACTED"/*url*/ << "\"" // Don't log secrets!
<< "\n");
return url;
}
catch (const std::exception& ex) {
// Workaround for what is probably a bug in SDAPI.
// Hopefully the dataset actually got closed anyway,
// so we don't get a resource leak here.
if (!strstr(ex.what(), "has been locked with different ID"))
throw;
_logger(0, "getSerializedContext() caused bogus exception for wid "+ wid + ": " + std::string(ex.what()));
_dataset->throwCloudException(ex, "altUrl");
throw; // not reached, but compiler might not realize that.
}
_logger(2, std::stringstream()
<< "SeismicStoreFile::altUrl(\""
<< (filename.size() < 76 ? filename : filename.substr(0, 72) + "...")
<< "\")"
<< " = \"" << "REDACTED"/*url*/ << "\"" // Don't log secrets!
<< "\n");
return url;
}
/**
......@@ -1782,8 +1863,13 @@ SeismicStoreFileDelayedWrite::SeismicStoreFileDelayedWrite(const std::string& fi
throw OpenZGY::Errors::ZgyUserError("Opening a file from seismic store requires a SeismicStoreIOContext");
this->_config.reset(new OpenZGY::SeismicStoreIOContext(*context));
if (mode == OpenMode::ReadWrite)
this->_reopen_last_segment();
try {
if (mode == OpenMode::ReadWrite)
this->_reopen_last_segment();
}
catch (const std::exception& ex) {
_relay->datasetwrapper()->throwCloudException(ex, "Open write");
}
}
SeismicStoreFileDelayedWrite::~SeismicStoreFileDelayedWrite()
......@@ -2092,6 +2178,8 @@ SeismicStoreFileDelayedWrite::xx_iscloud() const
* Called from constructor, so calling virtual methods in this class
* won't work.
*
* Catching and translating exceptions from SDAPI is done by caller.
*
* Thread safety: No.
*/
void
......
native/src/test/test_api.cpp
View file @ 5b569948
......@@ -2373,6 +2373,71 @@ test_hammer()
for (int ii = 0; ii < threads; ++ii)
workers[ii].join();
}
/**
* Most errors thrown by SDAPI now get caught and re-thrown as OpenZGY
* exceptions. In some cases OpenZGY is able to provide a better error
* message than SDAPI.
*/
static void
test_sderrors()
{
typedef OpenZGY::IZgyWriter::size3i_t size3i_t;
const std::string filename = cloud_synt2_name();
SeismicStoreIOContext context(*Test_Utils::default_sd_context());
// Neither token not token callback were provided.
context.sdtoken("", "");
must_throw("Missing access token or callback in iocontext", [&](){
OpenZGY::IZgyReader::open(filename, &context);
});
// Token callback provided but it always returns empty.
std::string token;
std::function<std::string()> functor = [&token]() {return token;};
context.sdtokencb(functor, "");
must_throw("Missing access token, first callback returned empty", [&](){
OpenZGY::IZgyReader::open(filename, &context);
});
#if 1 // Might or might not throw... So not much point in testing?
// Token callback provided but after a while it returns empty.
token = InternalZGY::Environment::getStringEnv("OPENZGY_TOKEN");
{
auto r = OpenZGY::IZgyReader::open(filename, &context);
float data{0};
r->read(size3i_t{0,0,0}, size3i_t{1,1,1}, &data, 0);
token = "";
// This doesn't necessarily throw, as the GCS token might be cached.
//must_throw("Missing access token, callback returned empty", [&](){
try {
float data2{0};
r->read(size3i_t{64,64,64}, size3i_t{1,1,1}, &data2, 0);
if (verbose())
std::cerr << "Step 1: NO exception" << std::endl;
}
catch (const std::exception& ex) {
if (verbose())
std::cerr << "Step 1: exception " << ex.what() << std::endl;
}
//});
// Ditto, might not throw.
//must_throw("Missing access token, callback returned empty", [&](){
try {
r->close();
if (verbose())
std::cerr << "Step 2: NO exception" << std::endl;
}
catch (const std::exception& ex) {
if (verbose())
std::cerr << "Step 2: exception " << ex.what() << std::endl;
}
//});
}
#endif
}
#endif
namespace {
......@@ -2786,6 +2851,7 @@ public:
#ifdef HAVE_SD
register_test("api.readwrite_cloud", test_readwrite_cloud);
register_test("api.hammer", test_hammer);
register_test("api.sderrors", test_sderrors);
#endif
register_test("api.edgebricks", test_edgebricks);
register_test("api.bat_local_1", test_bat_local_1);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment