Skip to content

Commit

Permalink
[Chromecast] Move SynchronizedMinidumpManager ratelimit logic to chil…
Browse files Browse the repository at this point in the history 
…d classes.

Previously rate limit logic was in SynchronizedMinidumpManager under the
assumption that this was the only producer for the lockfile.

This is not the case, so it makes more sense for ratelimit logic to be
contained in consumers of the lockfile.

TEST=modified tests pass
BUG=internal b/19210655

Review URL: https://codereview.chromium.org/1310313004

Cr-Commit-Position: refs/heads/master@{#348681}
  • Loading branch information
@baileyforrest
baileyforrest authored and Commit bot committed Sep 14, 2015
1 parent be04164 commit e2b5d24
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 154 deletions.
82 changes: 0 additions & 82 deletions chromecast/crash/linux/minidump_writer_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,86 +121,4 @@ TEST_F(MinidumpWriterTest, Write_FailsWithSubdirInCorrectPath) {
ASSERT_EQ(-1, writer.Write());
}

TEST_F(MinidumpWriterTest, Write_FailsWhenTooManyDumpsPresent) {
MinidumpWriter writer(&fake_generator_,
dumplog_file_.value(),
MinidumpParams(),
base::Bind(&FakeDumpState));

// Write dump logs to the lockfile.
size_t too_many_dumps = SynchronizedMinidumpManager::kMaxLockfileDumps + 1;
for (size_t i = 0; i < too_many_dumps; ++i) {
scoped_ptr<DumpInfo> info(CreateDumpInfo(
"{"
"\"name\": \"p\","
"\"dump_time\" : \"2012-01-01 01:02:03\","
"\"dump\": \"dump_string\","
"\"uptime\": \"123456789\","
"\"logfile\": \"logfile.log\""
"}"));
ASSERT_TRUE(info->valid());
ASSERT_TRUE(AppendLockFile(*info));
}

ASSERT_EQ(-1, writer.Write());
}

TEST_F(MinidumpWriterTest, Write_FailsWhenTooManyRecentDumpsPresent) {
MinidumpWriter writer(&fake_generator_,
dumplog_file_.value(),
MinidumpParams(),
base::Bind(&FakeDumpState));
// Multiple iters to make sure period resets work correctly
for (int iter = 0; iter < 3; ++iter) {
time_t now = time(nullptr);

// Write dump logs to the lockfile.
size_t too_many_recent_dumps =
SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps;
for (size_t i = 0; i < too_many_recent_dumps; ++i) {
ASSERT_EQ(0, writer.Write());

// Clear dumps so we don't reach max dumps in lockfile
ASSERT_TRUE(ClearDumps(lockfile_path_.value()));
}

// Should fail with too many dumps
ASSERT_EQ(-1, writer.Write());

int64 period = SynchronizedMinidumpManager::kRatelimitPeriodSeconds;

// Half period shouldn't trigger reset
SetRatelimitPeriodStart(metadata_path_.value(), now - period / 2);
ASSERT_EQ(-1, writer.Write());

// Set period starting time to trigger a reset
SetRatelimitPeriodStart(metadata_path_.value(), now - period);
}

ASSERT_EQ(0, writer.Write());
}

TEST_F(MinidumpWriterTest, Write_SucceedsWhenDumpLimitsNotExceeded) {
MinidumpWriter writer(&fake_generator_,
dumplog_file_.value(),
MinidumpParams(),
base::Bind(&FakeDumpState));

ASSERT_GT(SynchronizedMinidumpManager::kMaxLockfileDumps, 1);
ASSERT_GT(SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps, 0);

// Write an old dump logs to the lockfile.
scoped_ptr<DumpInfo> info(CreateDumpInfo(
"{"
"\"name\": \"p\","
"\"dump_time\" : \"2012-01-01 01:02:03\","
"\"dump\": \"dump_string\","
"\"uptime\": \"123456789\","
"\"logfile\": \"logfile.log\""
"}"));
ASSERT_TRUE(info->valid());
ASSERT_TRUE(AppendLockFile(*info));
ASSERT_EQ(0, writer.Write());
}

} // namespace chromecast
33 changes: 8 additions & 25 deletions chromecast/crash/linux/synchronized_minidump_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,16 +122,6 @@ int SetRatelimitPeriodDumps(base::Value* metadata, int period_dumps) {
return 0;
}

// Increment the number of dumps in the current ratelimit period in deserialized
// |metadata| by |increment|. Returns 0 on success, < 0 on error.
int IncrementCurrentPeriodDumps(base::Value* metadata, int increment) {
DCHECK_GE(increment, 0);
int last_dumps = GetRatelimitPeriodDumps(metadata);
RCHECK(last_dumps >= 0, -1);

return SetRatelimitPeriodDumps(metadata, last_dumps + increment);
}

// Returns true if |metadata| contains valid metadata, false otherwise.
bool ValidateMetadata(base::Value* metadata) {
RCHECK(metadata, false);
Expand All @@ -147,8 +137,6 @@ bool ValidateMetadata(base::Value* metadata) {

} // namespace

const int SynchronizedMinidumpManager::kMaxLockfileDumps = 5;

// One day
const int SynchronizedMinidumpManager::kRatelimitPeriodSeconds = 24 * 3600;
const int SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps = 100;
Expand Down Expand Up @@ -338,12 +326,6 @@ int SynchronizedMinidumpManager::AddEntryToLockFile(const DumpInfo& dump_info) {
return -1;
}

if (!CanWriteDumps(1)) {
LOG(ERROR) << "Can't Add Dump: Ratelimited";
return -1;
}

IncrementCurrentPeriodDumps(metadata_.get(), 1);
dumps_->Append(dump_info.GetAsValue());

return 0;
Expand Down Expand Up @@ -391,14 +373,15 @@ int SynchronizedMinidumpManager::SetCurrentDumps(
return 0;
}

bool SynchronizedMinidumpManager::CanWriteDumps(int num_dumps) {
const auto dumps(GetDumps());
int SynchronizedMinidumpManager::IncrementNumDumpsInCurrentPeriod() {
DCHECK(metadata_);
int last_dumps = GetRatelimitPeriodDumps(metadata_.get());
RCHECK(last_dumps >= 0, -1);

// If no more dumps can be written, return false.
if (static_cast<int>(dumps.size()) + num_dumps > kMaxLockfileDumps)
return false;
return SetRatelimitPeriodDumps(metadata_.get(), last_dumps + 1);
}

// If too many dumps have been written recently, return false.
bool SynchronizedMinidumpManager::CanUploadDump() {
time_t cur_time = time(nullptr);
time_t period_start = GetRatelimitPeriodStart(metadata_.get());
int period_dumps_count = GetRatelimitPeriodDumps(metadata_.get());
Expand All @@ -417,7 +400,7 @@ bool SynchronizedMinidumpManager::CanWriteDumps(int num_dumps) {
SetRatelimitPeriodDumps(metadata_.get(), period_dumps_count);
}

return period_dumps_count + num_dumps <= kRatelimitPeriodMaxDumps;
return period_dumps_count < kRatelimitPeriodMaxDumps;
}

} // namespace chromecast
19 changes: 9 additions & 10 deletions chromecast/crash/linux/synchronized_minidump_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,6 @@ namespace chromecast {
//
class SynchronizedMinidumpManager {
public:
// Max number of dumps allowed in lockfile.
static const int kMaxLockfileDumps;

// Length of a ratelimit period in seconds.
static const int kRatelimitPeriodSeconds;

Expand Down Expand Up @@ -100,6 +97,15 @@ class SynchronizedMinidumpManager {
// clean lingering dump files.
int GetNumDumps(bool delete_all_dumps);

// Increment the number of dumps in the current ratelimit period.
// Returns 0 on success, < 0 on error.
int IncrementNumDumpsInCurrentPeriod();

// Returns true when dumps uploaded in current rate limit period is less than
// |kRatelimitPeriodMaxDumps|. Resets rate limit period if period time has
// elapsed.
bool CanUploadDump();

// If true, the flock on the lockfile will be nonblocking.
bool non_blocking_;

Expand Down Expand Up @@ -127,13 +133,6 @@ class SynchronizedMinidumpManager {
// Release the lock file with the associated *fd*.
void ReleaseLockFile();

// Returns true if |num_dumps| can be written to the lockfile. We can write
// the dumps if the number of dumps in the lockfile after |num_dumps| is added
// is less than or equal to |kMaxLockfileDumps| and the number of dumps in the
// current ratelimit period after |num_dumps| is added is less than or equal
// to |kRatelimitPeriodMaxDumps|.
bool CanWriteDumps(int num_dumps);

const std::string lockfile_path_;
const std::string metadata_path_;
int lockfile_fd_;
Expand Down
127 changes: 90 additions & 37 deletions chromecast/crash/linux/synchronized_minidump_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ const char kMetadataName[] = "metadata";
const char kMinidumpSubdir[] = "minidumps";

// A trivial implementation of SynchronizedMinidumpManager, which does no work
// to the
// minidump and exposes its protected members for testing.
// to the minidump and exposes its protected members for testing. This simply
// adds an entry to the lockfile.
class SynchronizedMinidumpManagerSimple : public SynchronizedMinidumpManager {
public:
SynchronizedMinidumpManagerSimple()
Expand Down Expand Up @@ -77,6 +77,36 @@ void DoWorkLockedTask(SynchronizedMinidumpManagerSimple* manager) {
manager->DoWorkLocked();
}

// Simple SynchronizedMinidumpManager consumer. Checks if a dump can be uploaded
// then removes it from the lockfile.
class FakeSynchronizedMinidumpUploader : public SynchronizedMinidumpManager {
public:
FakeSynchronizedMinidumpUploader()
: SynchronizedMinidumpManager(), can_upload_return_val_(false) {}
~FakeSynchronizedMinidumpUploader() override {}

int DoWorkLocked() { return AcquireLockAndDoWork(); }

// SynchronizedMinidumpManager implementation:
int DoWork() override {
can_upload_return_val_ = CanUploadDump();

if (RemoveEntryFromLockFile(0) < 0)
return -1;

if (IncrementNumDumpsInCurrentPeriod() < 0)
return -1;

return 0;
}

// Accessors for testing.
bool can_upload_return_val() { return can_upload_return_val_; }

private:
bool can_upload_return_val_;
};

class SleepySynchronizedMinidumpManagerSimple
: public SynchronizedMinidumpManagerSimple {
public:
Expand Down Expand Up @@ -138,6 +168,23 @@ class SynchronizedMinidumpManagerTest : public testing::Test {
scoped_ptr<base::ScopedPathOverride> path_override_;
};

// Have |producer| generate |num_dumps| while checking there are no errors.
void produce_dumps(SynchronizedMinidumpManagerSimple& producer, int num_dumps) {
for (int i = 0; i < num_dumps; ++i) {
ASSERT_EQ(0, producer.DoWorkLocked());
ASSERT_EQ(0, producer.add_entry_return_code());
}
}

// Have |consumer| remove and process |num_dumps| while checking there are no
// errors.
void consume_dumps(FakeSynchronizedMinidumpUploader& consumer, int num_dumps) {
for (int i = 0; i < num_dumps; ++i) {
ASSERT_EQ(0, consumer.DoWorkLocked());
ASSERT_EQ(true, consumer.can_upload_return_val());
}
}

} // namespace

TEST_F(SynchronizedMinidumpManagerTest, FilePathsAreCorrect) {
Expand Down Expand Up @@ -384,72 +431,78 @@ TEST_F(SynchronizedMinidumpManagerTest,
}

TEST_F(SynchronizedMinidumpManagerTest,
AddEntryFailsWhenTooManyRecentDumpsPresent) {
Upload_SucceedsWhenDumpLimitsNotExceeded) {
// Sample parameters.
time_t now = time(0);
MinidumpParams params;
params.process_name = "process";

SynchronizedMinidumpManagerSimple manager;
manager.SetDumpInfoToWrite(
FakeSynchronizedMinidumpUploader uploader;
SynchronizedMinidumpManagerSimple producer;
producer.SetDumpInfoToWrite(
make_scoped_ptr(new DumpInfo("dump1", "log1", now, params)));

for (int i = 0; i < SynchronizedMinidumpManager::kMaxLockfileDumps; ++i) {
// Adding these should succeed
ASSERT_EQ(0, manager.DoWorkLocked());
ASSERT_EQ(0, manager.add_entry_return_code());
}
const int max_dumps = SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps;
produce_dumps(producer, max_dumps);
consume_dumps(uploader, max_dumps);
}

ASSERT_EQ(0, manager.DoWorkLocked());
TEST_F(SynchronizedMinidumpManagerTest, Upload_FailsWhenTooManyRecentDumps) {
// Sample parameters.
time_t now = time(0);
MinidumpParams params;
params.process_name = "process";

FakeSynchronizedMinidumpUploader uploader;
SynchronizedMinidumpManagerSimple producer;
producer.SetDumpInfoToWrite(
make_scoped_ptr(new DumpInfo("dump1", "log1", now, params)));

// This one should fail
ASSERT_GT(0, manager.add_entry_return_code());
const int max_dumps = SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps;
produce_dumps(producer, max_dumps + 1);
consume_dumps(uploader, max_dumps);

// Should fail with too many dumps
ASSERT_EQ(0, uploader.DoWorkLocked());
ASSERT_EQ(false, uploader.can_upload_return_val());
}

TEST_F(SynchronizedMinidumpManagerTest,
AddEntryFailsWhenRatelimitPeriodExceeded) {
TEST_F(SynchronizedMinidumpManagerTest, UploadSucceedsAfterRateLimitPeriodEnd) {
// Sample parameters.
time_t now = time(0);
MinidumpParams params;
params.process_name = "process";

SynchronizedMinidumpManagerSimple manager;
manager.SetDumpInfoToWrite(
FakeSynchronizedMinidumpUploader uploader;
SynchronizedMinidumpManagerSimple producer;
producer.SetDumpInfoToWrite(
make_scoped_ptr(new DumpInfo("dump1", "log1", now, params)));

// Multiple iters to make sure period resets work correctly
for (int iter = 0; iter < 3; ++iter) {
time_t now = time(nullptr);

// Write dump logs to the lockfile.
size_t too_many_recent_dumps =
SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps;
for (size_t i = 0; i < too_many_recent_dumps; ++i) {
// Adding these should succeed
ASSERT_EQ(0, manager.DoWorkLocked());
ASSERT_EQ(0, manager.add_entry_return_code());
const int iters = 3;
const int max_dumps = SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps;

// Clear dumps so we don't reach max dumps in lockfile
ASSERT_TRUE(ClearDumps(lockfile_.value()));
}
for (int i = 0; i < iters; ++i) {
produce_dumps(producer, max_dumps + 1);
consume_dumps(uploader, max_dumps);

ASSERT_EQ(0, manager.DoWorkLocked());
// Should fail with too many dumps
ASSERT_GT(0, manager.add_entry_return_code());
ASSERT_EQ(0, uploader.DoWorkLocked());
ASSERT_EQ(false, uploader.can_upload_return_val());

int64 period = SynchronizedMinidumpManager::kRatelimitPeriodSeconds;

// Half period shouldn't trigger reset
produce_dumps(producer, 1);
SetRatelimitPeriodStart(metadata_.value(), now - period / 2);
ASSERT_EQ(0, manager.DoWorkLocked());
ASSERT_GT(0, manager.add_entry_return_code());
ASSERT_EQ(0, uploader.DoWorkLocked());
ASSERT_EQ(false, uploader.can_upload_return_val());

// Set period starting time to trigger a reset
SetRatelimitPeriodStart(metadata_.value(), now - period);
}

ASSERT_EQ(0, manager.DoWorkLocked());
ASSERT_EQ(0, manager.add_entry_return_code());
produce_dumps(producer, 1);
consume_dumps(uploader, 1);
}

} // namespace chromecast

0 comments on commit e2b5d24

Please sign in to comment.