Skip to content

Commit

Permalink
Add synchronous callback support to important_file_writer.cc
Browse files Browse the repository at this point in the history 
This is required to support post-write callbacks which need to execute
even if Chrome is shutting down.

Carved out of https://codereview.chromium.org/2204943002/, new call will be used there.

BUG=624858

Review-Url: https://codereview.chromium.org/2299523003
Cr-Commit-Position: refs/heads/master@{#420102}
  • Loading branch information
proberge authored and Commit bot committed Sep 21, 2016
1 parent 37ac251 commit fc46ac1
Show file tree
Hide file tree
Showing 7 changed files with 427 additions and 97 deletions.
50 changes: 14 additions & 36 deletions base/files/important_file_writer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,13 @@ void LogFailure(const FilePath& path, TempFileFailure failure_code,

// Helper function to call WriteFileAtomically() with a
// std::unique_ptr<std::string>.
bool WriteScopedStringToFileAtomically(const FilePath& path,
std::unique_ptr<std::string> data) {
return ImportantFileWriter::WriteFileAtomically(path, *data);
void WriteScopedStringToFileAtomically(const FilePath& path,
std::unique_ptr<std::string> data,
Callback<void(bool success)> callback) {
bool result = ImportantFileWriter::WriteFileAtomically(path, *data);

if (!callback.is_null())
callback.Run(result);
}

} // namespace
Expand Down Expand Up @@ -168,8 +172,10 @@ void ImportantFileWriter::WriteNow(std::unique_ptr<std::string> data) {
if (HasPendingWrite())
timer_.Stop();

auto task = Bind(&WriteScopedStringToFileAtomically, path_, Passed(&data));
if (!PostWriteTask(task)) {
Closure task = Bind(&WriteScopedStringToFileAtomically, path_, Passed(&data),
Passed(&on_next_write_callback_));

if (!task_runner_->PostTask(FROM_HERE, MakeCriticalClosure(task))) {
// Posting the task to background message loop is not expected
// to fail, but if it does, avoid losing data and just hit the disk
// on the current thread.
Expand Down Expand Up @@ -203,37 +209,9 @@ void ImportantFileWriter::DoScheduledWrite() {
serializer_ = nullptr;
}

void ImportantFileWriter::RegisterOnNextSuccessfulWriteCallback(
const Closure& on_next_successful_write) {
DCHECK(on_next_successful_write_.is_null());
on_next_successful_write_ = on_next_successful_write;
}

bool ImportantFileWriter::PostWriteTask(const Callback<bool()>& task) {
// TODO(gab): This code could always use PostTaskAndReplyWithResult and let
// ForwardSuccessfulWrite() no-op if |on_next_successful_write_| is null, but
// PostTaskAndReply causes memory leaks in tests (crbug.com/371974) and
// suppressing all of those is unrealistic hence we avoid most of them by
// using PostTask() in the typical scenario below.
if (!on_next_successful_write_.is_null()) {
return PostTaskAndReplyWithResult(
task_runner_.get(),
FROM_HERE,
MakeCriticalClosure(task),
Bind(&ImportantFileWriter::ForwardSuccessfulWrite,
weak_factory_.GetWeakPtr()));
}
return task_runner_->PostTask(
FROM_HERE,
MakeCriticalClosure(Bind(IgnoreResult(task))));
}

void ImportantFileWriter::ForwardSuccessfulWrite(bool result) {
DCHECK(CalledOnValidThread());
if (result && !on_next_successful_write_.is_null()) {
on_next_successful_write_.Run();
on_next_successful_write_.Reset();
}
void ImportantFileWriter::RegisterOnNextWriteCallback(
const Callback<void(bool success)>& on_next_write_callback) {
on_next_write_callback_ = on_next_write_callback;
}

} // namespace base
24 changes: 11 additions & 13 deletions base/files/important_file_writer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,25 +94,23 @@ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe {
// Serialize data pending to be saved and execute write on backend thread.
void DoScheduledWrite();

// Registers |on_next_successful_write| to be called once, on the next
// successful write event. Only one callback can be set at once.
void RegisterOnNextSuccessfulWriteCallback(
const Closure& on_next_successful_write);
// Registers |on_next_write_callback| to be synchronously invoked from
// WriteFileAtomically() on its next write (i.e. from |task_runner_|), with
// |success| indicating whether it succeeded or not.
// |on_next_write_callback| must be thread safe, as it will be called on
// |task_runner_| and may be called during Chrome shutdown.
// If called more than once before a write is scheduled on |task_runner|, the
// latest callback clobbers the others.
void RegisterOnNextWriteCallback(
const Callback<void(bool success)>& on_next_write_callback);

TimeDelta commit_interval() const {
return commit_interval_;
}

private:
// Helper method for WriteNow().
bool PostWriteTask(const Callback<bool()>& task);

// If |result| is true and |on_next_successful_write_| is set, invokes
// |on_successful_write_| and then resets it; no-ops otherwise.
void ForwardSuccessfulWrite(bool result);

// Invoked once and then reset on the next successful write event.
Closure on_next_successful_write_;
// Invoked synchronously on the next write event.
Callback<void(bool success)> on_next_write_callback_;

// Path being written to.
const FilePath path_;
Expand Down
112 changes: 80 additions & 32 deletions base/files/important_file_writer_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,39 +46,45 @@ class DataSerializer : public ImportantFileWriter::DataSerializer {
const std::string data_;
};

class SuccessfulWriteObserver {
enum WriteCallbackObservationState {
NOT_CALLED,
CALLED_WITH_ERROR,
CALLED_WITH_SUCCESS,
};

class WriteCallbackObserver {
public:
SuccessfulWriteObserver() : successful_write_observed_(false) {}
WriteCallbackObserver() : observation_state_(NOT_CALLED) {}

// Register on_successful_write() to be called on the next successful write
// of |writer|.
void ObserveNextSuccessfulWrite(ImportantFileWriter* writer);
// Register OnWrite() to be called on the next write of |writer|.
void ObserveNextWriteCallback(ImportantFileWriter* writer);

// Returns true if a successful write was observed via on_successful_write()
// Returns true if a write was observed via OnWrite()
// and resets the observation state to false regardless.
bool GetAndResetObservationState();
WriteCallbackObservationState GetAndResetObservationState();

private:
void on_successful_write() {
EXPECT_FALSE(successful_write_observed_);
successful_write_observed_ = true;
void OnWrite(bool success) {
EXPECT_EQ(NOT_CALLED, observation_state_);
observation_state_ = success ? CALLED_WITH_SUCCESS : CALLED_WITH_ERROR;
}

bool successful_write_observed_;
WriteCallbackObservationState observation_state_;

DISALLOW_COPY_AND_ASSIGN(SuccessfulWriteObserver);
DISALLOW_COPY_AND_ASSIGN(WriteCallbackObserver);
};

void SuccessfulWriteObserver::ObserveNextSuccessfulWrite(
void WriteCallbackObserver::ObserveNextWriteCallback(
ImportantFileWriter* writer) {
writer->RegisterOnNextSuccessfulWriteCallback(base::Bind(
&SuccessfulWriteObserver::on_successful_write, base::Unretained(this)));
writer->RegisterOnNextWriteCallback(
base::Bind(&WriteCallbackObserver::OnWrite, base::Unretained(this)));
}

bool SuccessfulWriteObserver::GetAndResetObservationState() {
bool was_successful_write_observed = successful_write_observed_;
successful_write_observed_ = false;
return was_successful_write_observed;
WriteCallbackObservationState
WriteCallbackObserver::GetAndResetObservationState() {
WriteCallbackObservationState state = observation_state_;
observation_state_ = NOT_CALLED;
return state;
}

} // namespace
Expand All @@ -92,7 +98,7 @@ class ImportantFileWriterTest : public testing::Test {
}

protected:
SuccessfulWriteObserver successful_write_observer_;
WriteCallbackObserver write_callback_observer_;
FilePath file_;
MessageLoop loop_;

Expand All @@ -103,49 +109,91 @@ class ImportantFileWriterTest : public testing::Test {
TEST_F(ImportantFileWriterTest, Basic) {
ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get());
EXPECT_FALSE(PathExists(writer.path()));
EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
writer.WriteNow(MakeUnique<std::string>("foo"));
RunLoop().RunUntilIdle();

EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
ASSERT_TRUE(PathExists(writer.path()));
EXPECT_EQ("foo", GetFileContent(writer.path()));
}

TEST_F(ImportantFileWriterTest, BasicWithSuccessfulWriteObserver) {
TEST_F(ImportantFileWriterTest, WriteWithObserver) {
ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get());
EXPECT_FALSE(PathExists(writer.path()));
EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());

// Confirm that the observer is invoked.
write_callback_observer_.ObserveNextWriteCallback(&writer);
writer.WriteNow(MakeUnique<std::string>("foo"));
RunLoop().RunUntilIdle();

// Confirm that the observer is invoked.
EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
EXPECT_EQ(CALLED_WITH_SUCCESS,
write_callback_observer_.GetAndResetObservationState());
ASSERT_TRUE(PathExists(writer.path()));
EXPECT_EQ("foo", GetFileContent(writer.path()));

// Confirm that re-installing the observer works for another write.
EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
write_callback_observer_.ObserveNextWriteCallback(&writer);
writer.WriteNow(MakeUnique<std::string>("bar"));
RunLoop().RunUntilIdle();

EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
EXPECT_EQ(CALLED_WITH_SUCCESS,
write_callback_observer_.GetAndResetObservationState());
ASSERT_TRUE(PathExists(writer.path()));
EXPECT_EQ("bar", GetFileContent(writer.path()));

// Confirm that writing again without re-installing the observer doesn't
// result in a notification.
EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
writer.WriteNow(MakeUnique<std::string>("baz"));
RunLoop().RunUntilIdle();

EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
ASSERT_TRUE(PathExists(writer.path()));
EXPECT_EQ("baz", GetFileContent(writer.path()));
}

TEST_F(ImportantFileWriterTest, FailedWriteWithObserver) {
// Use an invalid file path (relative paths are invalid) to get a
// FILE_ERROR_ACCESS_DENIED error when trying to write the file.
ImportantFileWriter writer(FilePath().AppendASCII("bad/../path"),
ThreadTaskRunnerHandle::Get());
EXPECT_FALSE(PathExists(writer.path()));
EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
write_callback_observer_.ObserveNextWriteCallback(&writer);
writer.WriteNow(MakeUnique<std::string>("foo"));
RunLoop().RunUntilIdle();

// Confirm that the write observer was invoked with its boolean parameter set
// to false.
EXPECT_EQ(CALLED_WITH_ERROR,
write_callback_observer_.GetAndResetObservationState());
EXPECT_FALSE(PathExists(writer.path()));
}

TEST_F(ImportantFileWriterTest, CallbackRunsOnWriterThread) {
base::Thread file_writer_thread("ImportantFileWriter test thread");
file_writer_thread.Start();
ImportantFileWriter writer(file_, file_writer_thread.task_runner());
EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());

write_callback_observer_.ObserveNextWriteCallback(&writer);
writer.WriteNow(MakeUnique<std::string>("foo"));
RunLoop().RunUntilIdle();

// Expect the callback to not have been executed before the write.
EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());

// Make sure tasks posted by WriteNow() have ran before continuing.
file_writer_thread.FlushForTesting();
EXPECT_EQ(CALLED_WITH_SUCCESS,
write_callback_observer_.GetAndResetObservationState());
ASSERT_TRUE(PathExists(writer.path()));
EXPECT_EQ("foo", GetFileContent(writer.path()));
}

TEST_F(ImportantFileWriterTest, ScheduleWrite) {
ImportantFileWriter writer(file_,
ThreadTaskRunnerHandle::Get(),
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/prefs/profile_pref_store_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,9 @@ PersistentPrefStore* ProfilePrefStoreManager::CreateProfilePrefStore(
unprotected_pref_names, protected_pref_names,
base::Bind(&RemoveValueSilently, unprotected_pref_store->AsWeakPtr()),
base::Bind(&RemoveValueSilently, protected_pref_store->AsWeakPtr()),
base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteCallback,
base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteReply,
unprotected_pref_store->AsWeakPtr()),
base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteCallback,
base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteReply,
protected_pref_store->AsWeakPtr()),
GetPrefHashStore(false), GetPrefHashStore(true),
raw_unprotected_pref_hash_filter, raw_protected_pref_hash_filter);
Expand Down
67 changes: 64 additions & 3 deletions components/prefs/json_pref_store.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
#include "base/task_runner_util.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "base/threading/sequenced_worker_pool.h"
#include "base/time/default_clock.h"
#include "base/values.h"
Expand Down Expand Up @@ -171,6 +172,8 @@ JsonPrefStore::JsonPrefStore(
filtering_in_progress_(false),
pending_lossy_write_(false),
read_error_(PREF_READ_ERROR_NONE),
has_pending_successful_write_reply_(false),
has_pending_write_callback_(false),
write_count_histogram_(writer_.commit_interval(), path_) {
DCHECK(!path_.empty());
}
Expand Down Expand Up @@ -323,11 +326,69 @@ void JsonPrefStore::ReportValueChanged(const std::string& key, uint32_t flags) {
ScheduleWrite(flags);
}

void JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(
const base::Closure& on_next_successful_write) {
void JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback(
bool write_success) {
DCHECK(CalledOnValidThread());

writer_.RegisterOnNextSuccessfulWriteCallback(on_next_successful_write);
has_pending_write_callback_ = false;
if (has_pending_successful_write_reply_) {
has_pending_successful_write_reply_ = false;
if (write_success) {
on_next_successful_write_reply_.Run();
} else {
RegisterOnNextSuccessfulWriteReply(on_next_successful_write_reply_);
}
}
}

// static
void JsonPrefStore::PostWriteCallback(
const base::Callback<void(bool success)>& on_next_write_reply,
const base::Callback<void(bool success)>& on_next_write_callback,
scoped_refptr<base::SequencedTaskRunner> reply_task_runner,
bool write_success) {
if (!on_next_write_callback.is_null())
on_next_write_callback.Run(write_success);

// We can't run |on_next_write_reply| on the current thread. Bounce back to
// the |reply_task_runner| which is the correct sequenced thread.
reply_task_runner->PostTask(FROM_HERE,
base::Bind(on_next_write_reply, write_success));
}

void JsonPrefStore::RegisterOnNextSuccessfulWriteReply(
const base::Closure& on_next_successful_write_reply) {
DCHECK(CalledOnValidThread());
DCHECK(!has_pending_successful_write_reply_);

has_pending_successful_write_reply_ = true;
on_next_successful_write_reply_ = on_next_successful_write_reply;

// If there already is a pending callback, avoid erasing it; the reply will
// be used as we set |on_next_successful_write_reply_|. Otherwise, setup a
// reply with an empty callback.
if (!has_pending_write_callback_) {
writer_.RegisterOnNextWriteCallback(base::Bind(
&PostWriteCallback,
base::Bind(&JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback,
AsWeakPtr()),
base::Callback<void(bool success)>(),
base::SequencedTaskRunnerHandle::Get()));
}
}

void JsonPrefStore::RegisterOnNextWriteSynchronousCallback(
const base::Callback<void(bool success)>& on_next_write_callback) {
DCHECK(CalledOnValidThread());
DCHECK(!has_pending_write_callback_);

has_pending_write_callback_ = true;

writer_.RegisterOnNextWriteCallback(base::Bind(
&PostWriteCallback,
base::Bind(&JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback,
AsWeakPtr()),
on_next_write_callback, base::SequencedTaskRunnerHandle::Get()));
}

void JsonPrefStore::ClearMutableValues() {
Expand Down
Loading

0 comments on commit fc46ac1

Please sign in to comment.