diff options
author | Cassie Wang <cassiewang@google.com> | 2021-07-07 16:06:44 -0700 |
---|---|---|
committer | Cassie Wang <cassiewang@google.com> | 2021-07-08 11:44:19 -0700 |
commit | 9bc40e383c7a040ac343b5b14b46e53878fc0f06 (patch) | |
tree | f5a066d90c489bfd8d2d084700f20cf9b138aee8 | |
parent | 8ed9ccef5cd49d935a322118c8697da411f4ae51 (diff) | |
download | icing-9bc40e383c7a040ac343b5b14b46e53878fc0f06.tar.gz |
Fix Reset() calls from crashing.
Properly resets the unique_ptr instead of just reassigning them.
Bug: 193052798
Test: presubmit
Change-Id: I93e12579d3244d8243eb5f56860dee82d00b4ad7
-rw-r--r-- | icing/icing-search-engine.cc | 24 | ||||
-rw-r--r-- | icing/icing-search-engine_test.cc | 56 |
2 files changed, 22 insertions, 58 deletions
diff --git a/icing/icing-search-engine.cc b/icing/icing-search-engine.cc index cf998bf..20a6bb9 100644 --- a/icing/icing-search-engine.cc +++ b/icing/icing-search-engine.cc @@ -1662,24 +1662,22 @@ ResetResultProto IcingSearchEngine::Reset() { ResetResultProto result_proto; StatusProto* result_status = result_proto.mutable_status(); - int64_t before_size = filesystem_->GetDiskUsage(options_.base_dir().c_str()); + absl_ports::unique_lock l(&mutex_); - if (!filesystem_->DeleteDirectoryRecursively(options_.base_dir().c_str())) { - int64_t after_size = filesystem_->GetDiskUsage(options_.base_dir().c_str()); - if (after_size != before_size) { - // Our filesystem doesn't atomically delete. If we have a discrepancy in - // size, then that means we may have deleted some files, but not others. - // So our data is in an invalid state now. - result_status->set_code(StatusProto::INTERNAL); - return result_proto; - } + initialized_ = false; - result_status->set_code(StatusProto::ABORTED); + // Resets members variables + schema_store_.reset(); + document_store_.reset(); + language_segmenter_.reset(); + normalizer_.reset(); + index_.reset(); + + if (!filesystem_->DeleteDirectoryRecursively(options_.base_dir().c_str())) { + result_status->set_code(StatusProto::INTERNAL); return result_proto; } - absl_ports::unique_lock l(&mutex_); - initialized_ = false; if (InternalInitialize().status().code() != StatusProto::OK) { // We shouldn't hit the following Initialize errors: // NOT_FOUND: all data was cleared, we aren't expecting anything diff --git a/icing/icing-search-engine_test.cc b/icing/icing-search-engine_test.cc index 5760614..4c15827 100644 --- a/icing/icing-search-engine_test.cc +++ b/icing/icing-search-engine_test.cc @@ -5532,11 +5532,11 @@ TEST_F(IcingSearchEngineTest, ResetOk) { EXPECT_THAT(icing.SetSchema(empty_schema).status(), ProtoIsOk()); } -TEST_F(IcingSearchEngineTest, ResetDeleteFailureCausesAbortedError) { +TEST_F(IcingSearchEngineTest, ResetDeleteFailureCausesInternalError) { auto mock_filesystem = std::make_unique<MockFilesystem>(); - // This fails IcingSearchEngine::Reset(). But since we didn't actually delete - // anything, we'll be able to consider this just an ABORTED call. + // This fails IcingSearchEngine::Reset() with status code INTERNAL and leaves + // the IcingSearchEngine instance in an uninitialized state. ON_CALL(*mock_filesystem, DeleteDirectoryRecursively(StrEq(GetTestBaseDir().c_str()))) .WillByDefault(Return(false)); @@ -5550,51 +5550,17 @@ TEST_F(IcingSearchEngineTest, ResetDeleteFailureCausesAbortedError) { DocumentProto document = CreateMessageDocument("namespace", "uri"); ASSERT_THAT(icing.Put(document).status(), ProtoIsOk()); - EXPECT_THAT(icing.Reset().status(), ProtoStatusIs(StatusProto::ABORTED)); + EXPECT_THAT(icing.Reset().status(), ProtoStatusIs(StatusProto::INTERNAL)); - // Everything is still intact. - // Can get old data. GetResultProto expected_get_result_proto; - expected_get_result_proto.mutable_status()->set_code(StatusProto::OK); + expected_get_result_proto.mutable_status()->set_code( + StatusProto::FAILED_PRECONDITION); *expected_get_result_proto.mutable_document() = document; - EXPECT_THAT(icing.Get(document.namespace_(), document.uri(), - GetResultSpecProto::default_instance()), - EqualsProto(expected_get_result_proto)); - - // Can add new data. - EXPECT_THAT(icing.Put(CreateMessageDocument("namespace", "uri")).status(), - ProtoIsOk()); -} - -TEST_F(IcingSearchEngineTest, ResetCreateFailureCausesInternalError) { - auto mock_filesystem = std::make_unique<MockFilesystem>(); - - // Let all other delete directory calls succeed. - EXPECT_CALL(*mock_filesystem, - DeleteDirectoryRecursively(Matcher<const char*>(_))) - .WillRepeatedly(Return(true)); - - // This prevents IcingSearchEngine from deleting our base dir when resetting - EXPECT_CALL(*mock_filesystem, DeleteDirectoryRecursively(Matcher<const char*>( - StrEq(GetTestBaseDir().c_str())))) - .WillOnce(Return(false)); - - // The first call will show our base directory had 100 bytes, but after we - // falied to delete, we lost those 100 bytes. So this will be reported as an - // INTERNAL error since data was lost. - EXPECT_CALL( - *mock_filesystem, - GetDiskUsage(Matcher<const char*>(StrEq(GetTestBaseDir().c_str())))) - .WillOnce(Return(100)) - .WillOnce(Return(0)); - - TestIcingSearchEngine icing(GetDefaultIcingOptions(), - std::move(mock_filesystem), - std::make_unique<IcingFilesystem>(), - std::make_unique<FakeClock>(), GetTestJniCache()); - ASSERT_THAT(icing.Initialize().status(), ProtoIsOk()); - ASSERT_THAT(icing.SetSchema(CreateMessageSchema()).status(), ProtoIsOk()); - EXPECT_THAT(icing.Reset().status(), ProtoStatusIs(StatusProto::INTERNAL)); + EXPECT_THAT(icing + .Get(document.namespace_(), document.uri(), + GetResultSpecProto::default_instance()) + .status(), + ProtoStatusIs(StatusProto::FAILED_PRECONDITION)); } TEST_F(IcingSearchEngineTest, SnippetNormalization) { |