Fix a timer crash caused by invalid memory management #9656

jay-zhuang · 2022-03-03T21:05:04Z

Timer crash when multiple DB instances doing heavy DB open and close
operations concurrently. Which is caused by adding a timer task with
smaller timestamp than the current running task. Fix it by moving the
getting new task timestamp part within timer mutex protection.
And other fixes:

Disallow adding duplicated function name to timer
Fix a minor memory leak in timer when a running task is cancelled

facebook-github-bot · 2022-03-03T22:31:16Z

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

riversand963

Good catch and thanks @jay-zhuang for the fix! Overall LGTM except a few minor comments/questions.

riversand963 · 2022-03-07T16:50:40Z

util/timer.h

+    auto it = map_.find(fn_name);
+    if (it == map_.end()) {
+      heap_.push(fn_info.get());
+      map_.emplace(std::make_pair(fn_name, std::move(fn_info)));


We can just do map_.emplace(fn_name, std::move(fn_info)).

riversand963 · 2022-03-07T17:04:16Z

util/timer.h

-           const std::string& fn_name,
-           uint64_t start_after_us,
-           uint64_t repeat_every_us) {
+  bool Add(std::function<void()> fn, const std::string& fn_name,


I think we should update the comments in line 51. Specifically, fn_name has to be unique, and we disallow adding function with the same name to timer.

riversand963 · 2022-03-07T17:05:33Z

util/timer.h

+    auto it = map_.find(fn_name);
+    if (it == map_.end()) {
+      heap_.push(fn_info.get());


We can use try_emplace()

I guess you mean map_.try_emplace(), updated.

Sorry, I mean something like

auto res = map_.try_emplace(fn_name, std::move(fn_info)); if (!res.second) { return false; } heap_.push(res.first.second.get());

riversand963 · 2022-03-07T17:13:46Z

util/timer.h

-           uint64_t repeat_every_us) {
+  bool Add(std::function<void()> fn, const std::string& fn_name,
+           uint64_t start_after_us, uint64_t repeat_every_us) {
+    InstrumentedMutexLock l(&mutex_);


Nit: we can avoid dynamic memory allocation inside this mutex by calling NowMicros() and setting next run time after mutex acquision.

std::unique_ptr<> fn_info(new FunctionInfo(..., dummy_time,...)); InstrumentedMutexLock l(&mutex_); now = NowMicros(); fn_info->SetNextRunTime(now + start_after_us); ...

riversand963 · 2022-03-07T17:24:27Z

util/timer.h

+    assert(!executing_task_ ||
+           fn_info->next_run_time_us >= heap_.top()->next_run_time_us);


It seems we should return false here instead of aborting the process if we hit this condition?

facebook-github-bot · 2022-03-08T00:54:55Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

riversand963

Thanks @jay-zhuang. LGTM except a minor comment and one of the CIs complaining about unused var.

riversand963 · 2022-03-08T17:14:49Z

util/timer.h

+    auto it = map_.find(fn_name);
+    if (it == map_.end()) {
+      heap_.push(fn_info.get());


Sorry, I mean something like

auto res = map_.try_emplace(fn_name, std::move(fn_info)); if (!res.second) { return false; } heap_.push(res.first.second.get());

ajkr

Wanted to confirm my understanding: Is the problem we remove the same name twice from map_ and the second time fails? If so, does the duplicate entry come from popping the wrong thing from heap_ due to something added with earlier running time than the executing function? Then we re-add an existing thing to heap_ which would cause two map_.erase()s after it's Cancel()d?

Anyways the new code to keep heap_ and map_ in sync looks great!

jay-zhuang · 2022-03-11T21:58:14Z

Wanted to confirm my understanding: Is the problem we remove the same name twice from map_ and the second time fails?

Yes, but it fails before we try to remove the item the second time as it's memory already be released including the string current_fn->name. So it failed to get the fn->name. not fail to map_.erase().

If so, does the duplicate entry come from popping the wrong thing from heap_ due to something added with earlier running time than the executing function? Then we re-add an existing thing to heap_ which would cause two map_.erase()s after it's Cancel()d?

Yes, the duplicated entry come from popping the wrong item (it suppose to be popping the current running one then update the next_run_time and re-add it back. but in this case, it's popping the new added one as it's timestamp is newer than the current running one, so after that the current running one is duplicated in the queue).

facebook-github-bot · 2022-03-12T00:19:54Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

Timer crash when multiple DB instances doing heavy DB open and close operations concurrently. Which is caused by adding a timer task with smaller timestamp than the current running task. Fix it by moving the getting new task timestamp part within timer mutex protection. And other fixes: - Disallow adding duplicated function number to timer - Fix a memory leak in timer when current running task is cancelled

facebook-github-bot · 2022-03-12T17:52:04Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-03-12T17:56:22Z

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Timer crash when multiple DB instances doing heavy DB open and close operations concurrently. Which is caused by adding a timer task with smaller timestamp than the current running task. Fix it by moving the getting new task timestamp part within timer mutex protection. And other fixes: - Disallow adding duplicated function name to timer - Fix a minor memory leak in timer when a running task is cancelled Pull Request resolved: #9656 Reviewed By: ajkr Differential Revision: D34626296 Pulled By: jay-zhuang fbshipit-source-id: 6b6d96a5149746bf503546244912a9e41a0c5f6b

wangxipu · 2022-11-30T03:38:10Z

db/compaction/compaction_picker_universal.cc

@@ -372,6 +372,7 @@ Compaction* UniversalCompactionBuilder::PickCompaction() {
  const int kLevel0 = 0;
  score_ = vstorage_->CompactionScore(kLevel0);
  sorted_runs_ = CalculateSortedRuns(*vstorage_);
+  fprintf(stdout, "JJJ1\n");


delete test code？

facebook-github-bot added the CLA Signed label Mar 3, 2022

jay-zhuang mentioned this pull request Mar 3, 2022

Rocks DB crash when used via JNI. Version: 6.20.3 #8617

Closed

jay-zhuang marked this pull request as ready for review March 3, 2022 22:31

jay-zhuang requested a review from riversand963 March 3, 2022 22:31

jay-zhuang linked an issue Mar 4, 2022 that may be closed by this pull request

Rocks DB crash when used via JNI. Version: 6.20.3 #8617

Closed

riversand963 reviewed Mar 7, 2022

View reviewed changes

jay-zhuang force-pushed the timer_fix2 branch from c6ba794 to c85eb51 Compare March 8, 2022 00:54

riversand963 approved these changes Mar 8, 2022

View reviewed changes

ajkr approved these changes Mar 8, 2022

View reviewed changes

jay-zhuang force-pushed the timer_fix2 branch from c85eb51 to c0465aa Compare March 12, 2022 00:19

jay-zhuang added 6 commits March 12, 2022 09:51

fix lite build

ec5e605

review feedbacks

daf7144

debug

cf00891

Fix gcc7 build

d90ee2f

make format

5e9ca8a

jay-zhuang force-pushed the timer_fix2 branch from c0465aa to 5e9ca8a Compare March 12, 2022 17:52

facebook-github-bot closed this in 09b0e8f Mar 12, 2022

jay-zhuang added a commit to jay-zhuang/rocksdb that referenced this pull request Mar 13, 2022

Remove debug from facebook#9656

ce243df

ajkr mentioned this pull request Mar 29, 2022

Status of rocksdb 6? #9768

Closed

wangxipu reviewed Nov 30, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a timer crash caused by invalid memory management #9656

Fix a timer crash caused by invalid memory management #9656

jay-zhuang commented Mar 3, 2022 •

edited

Loading

facebook-github-bot commented Mar 3, 2022

riversand963 left a comment

riversand963 Mar 7, 2022

riversand963 Mar 7, 2022

riversand963 Mar 7, 2022

jay-zhuang Mar 8, 2022

riversand963 Mar 8, 2022

riversand963 Mar 7, 2022

riversand963 Mar 7, 2022

facebook-github-bot commented Mar 8, 2022

riversand963 left a comment

riversand963 Mar 8, 2022

ajkr left a comment •

edited

Loading

jay-zhuang commented Mar 11, 2022

facebook-github-bot commented Mar 12, 2022

facebook-github-bot commented Mar 12, 2022

facebook-github-bot commented Mar 12, 2022

wangxipu Nov 30, 2022

		assert(!executing_task_ \|\|
		fn_info->next_run_time_us >= heap_.top()->next_run_time_us);

Fix a timer crash caused by invalid memory management #9656

Fix a timer crash caused by invalid memory management #9656

Conversation

jay-zhuang commented Mar 3, 2022 • edited Loading

facebook-github-bot commented Mar 3, 2022

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 8, 2022

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajkr left a comment • edited Loading

Choose a reason for hiding this comment

jay-zhuang commented Mar 11, 2022

facebook-github-bot commented Mar 12, 2022

facebook-github-bot commented Mar 12, 2022

facebook-github-bot commented Mar 12, 2022

Choose a reason for hiding this comment

jay-zhuang commented Mar 3, 2022 •

edited

Loading

ajkr left a comment •

edited

Loading