Skip to content

Commit

Permalink
prototype status check enforcement (#6798)
Browse files Browse the repository at this point in the history 
Summary:
Tried making Status object enforce that it is checked in some way. In cases it is not checked, `PermitUncheckedError()` must be called explicitly.

Added a way to run tests (`ASSERT_STATUS_CHECKED=1 make -j48 check`) on a
whitelist. The effort appears significant to get each test to pass with
this assertion, so I only fixed up enough to get one test (`options_test`)
working and added it to the whitelist.
Pull Request resolved: #6798

Reviewed By: pdillinger

Differential Revision: D21377404

Pulled By: ajkr

fbshipit-source-id: 73236f9c8df38f01cf24ecac4a6d1661b72d077e
  • Loading branch information
@ajkr @facebook-github-bot
ajkr authored and facebook-github-bot committed May 8, 2020
1 parent 1282589 commit 1c84660
Show file tree
Hide file tree
Showing 11 changed files with 389 additions and 125 deletions.
14 changes: 14 additions & 0 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,12 @@ else
CXXFLAGS += -fno-rtti
endif

ifdef ASSERT_STATUS_CHECKED
ifeq ($(filter -DROCKSDB_ASSERT_STATUS_CHECKED,$(OPT)),)
OPT += -DROCKSDB_ASSERT_STATUS_CHECKED
endif
endif

$(warning Warning: Compiling in debug mode. Don't use the resulting binary in production)
endif

Expand Down Expand Up @@ -657,6 +663,14 @@ PARALLEL_TEST = \
ifdef COMPILE_WITH_UBSAN
TESTS := $(shell echo $(TESTS) | sed 's/\boptions_settable_test\b//g')
endif
ifdef ASSERT_STATUS_CHECKED
# This is a new check for which we will add support incrementally. The
# whitelist can be removed once support is fully added.
TESTS_WHITELIST = \
options_test
TESTS := $(filter $(TESTS_WHITELIST),$(TESTS))
PARALLEL_TEST := $(filter $(TESTS_WHITELIST),$(PARALLEL_TEST))
endif
SUBSET := $(TESTS)
ifdef ROCKSDBTESTS_START
SUBSET := $(shell echo $(SUBSET) | sed 's/^.*$(ROCKSDBTESTS_START)/$(ROCKSDBTESTS_START)/')
Expand Down
6 changes: 5 additions & 1 deletion file/writable_file_writer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class WritableFileWriter {
for (auto& listener : listeners_) {
listener->OnFileWriteFinish(info);
}
info.status.PermitUncheckedError();
}
#endif // ROCKSDB_LITE

Expand Down Expand Up @@ -126,7 +127,10 @@ class WritableFileWriter {

WritableFileWriter& operator=(const WritableFileWriter&) = delete;

~WritableFileWriter() { Close(); }
~WritableFileWriter() {
auto s = Close();
s.PermitUncheckedError();
}

std::string file_name() const { return file_name_; }

Expand Down
46 changes: 23 additions & 23 deletions hdfs/env_hdfs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,6 @@ class HdfsEnv : public Env {

namespace ROCKSDB_NAMESPACE {

static const Status notsup;

class HdfsEnv : public Env {

public:
Expand All @@ -260,79 +258,81 @@ class HdfsEnv : public Env {
const std::string& /*fname*/,
std::unique_ptr<RandomAccessFile>* /*result*/,
const EnvOptions& /*options*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status NewWritableFile(const std::string& /*fname*/,
std::unique_ptr<WritableFile>* /*result*/,
const EnvOptions& /*options*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status NewDirectory(const std::string& /*name*/,
std::unique_ptr<Directory>* /*result*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status FileExists(const std::string& /*fname*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status GetChildren(const std::string& /*path*/,
std::vector<std::string>* /*result*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status DeleteFile(const std::string& /*fname*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status CreateDir(const std::string& /*name*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status CreateDirIfMissing(const std::string& /*name*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status DeleteDir(const std::string& /*name*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status GetFileSize(const std::string& /*fname*/,
uint64_t* /*size*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status GetFileModificationTime(const std::string& /*fname*/,
uint64_t* /*time*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status RenameFile(const std::string& /*src*/,
const std::string& /*target*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status LinkFile(const std::string& /*src*/,
const std::string& /*target*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status LockFile(const std::string& /*fname*/,
FileLock** /*lock*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status UnlockFile(FileLock* /*lock*/) override { return notsup; }
virtual Status UnlockFile(FileLock* /*lock*/) override {
return Status::NotSupported();
}

virtual Status NewLogger(const std::string& /*fname*/,
std::shared_ptr<Logger>* /*result*/) override {
return notsup;
return Status::NotSupported();
}

Status IsDirectory(const std::string& /*path*/, bool* /*is_dir*/) override {
return notsup;
return Status::NotSupported();
}

virtual void Schedule(void (* /*function*/)(void* arg), void* /*arg*/,
Expand All @@ -352,24 +352,24 @@ class HdfsEnv : public Env {
}

virtual Status GetTestDirectory(std::string* /*path*/) override {
return notsup;
return Status::NotSupported();
}

virtual uint64_t NowMicros() override { return 0; }

virtual void SleepForMicroseconds(int /*micros*/) override {}

virtual Status GetHostName(char* /*name*/, uint64_t /*len*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status GetCurrentTime(int64_t* /*unix_time*/) override {
return notsup;
return Status::NotSupported();
}

virtual Status GetAbsolutePath(const std::string& /*db_path*/,
std::string* /*outputpath*/) override {
return notsup;
return Status::NotSupported();
}

virtual void SetBackgroundThreads(int /*number*/,
Expand Down
21 changes: 20 additions & 1 deletion include/rocksdb/io_status.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@ inline IOStatus::IOStatus(Code _code, SubCode _subcode, const Slice& msg,
}

inline IOStatus::IOStatus(const IOStatus& s) : Status(s.code_, s.subcode_) {
#ifdef ROCKSDB_ASSERT_STATUS_CHECKED
s.checked_ = true;
#endif // ROCKSDB_ASSERT_STATUS_CHECKED
retryable_ = s.retryable_;
data_loss_ = s.data_loss_;
scope_ = s.scope_;
Expand All @@ -179,6 +182,10 @@ inline IOStatus& IOStatus::operator=(const IOStatus& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are ok.
if (this != &s) {
#ifdef ROCKSDB_ASSERT_STATUS_CHECKED
s.checked_ = true;
checked_ = false;
#endif // ROCKSDB_ASSERT_STATUS_CHECKED
code_ = s.code_;
subcode_ = s.subcode_;
retryable_ = s.retryable_;
Expand All @@ -204,14 +211,18 @@ inline IOStatus& IOStatus::operator=(IOStatus&& s)
#endif
{
if (this != &s) {
#ifdef ROCKSDB_ASSERT_STATUS_CHECKED
s.checked_ = true;
checked_ = false;
#endif // ROCKSDB_ASSERT_STATUS_CHECKED
code_ = std::move(s.code_);
s.code_ = kOk;
subcode_ = std::move(s.subcode_);
s.subcode_ = kNone;
retryable_ = s.retryable_;
data_loss_ = s.data_loss_;
scope_ = s.scope_;
scope_ = kIOErrorScopeFileSystem;
s.scope_ = kIOErrorScopeFileSystem;
delete[] state_;
state_ = nullptr;
std::swap(state_, s.state_);
Expand All @@ -220,10 +231,18 @@ inline IOStatus& IOStatus::operator=(IOStatus&& s)
}

inline bool IOStatus::operator==(const IOStatus& rhs) const {
#ifdef ROCKSDB_ASSERT_STATUS_CHECKED
checked_ = true;
rhs.checked_ = true;
#endif // ROCKSDB_ASSERT_STATUS_CHECKED
return (code_ == rhs.code_);
}

inline bool IOStatus::operator!=(const IOStatus& rhs) const {
#ifdef ROCKSDB_ASSERT_STATUS_CHECKED
checked_ = true;
rhs.checked_ = true;
#endif // ROCKSDB_ASSERT_STATUS_CHECKED
return !(*this == rhs);
}

Expand Down
Loading

0 comments on commit 1c84660

Please sign in to comment.