Skip to content

feat: support to close percentile to prevent from heap-use-after-free error - #1074

Merged
empiredan merged 14 commits into
apache:masterfrom
empiredan:support-close-percentile
Jul 28, 2022
Merged

empiredan merged 14 commits into
apache:masterfrom
empiredan:support-close-percentile

Conversation

@empiredan

@empiredan empiredan commented Jul 24, 2022 •

Copy link
Copy Markdown
Contributor

This PR is to resolve the issue #1066.

The root cause of issue #1066 is that the percentile is freed during the destruction of the metric registry, after which sometimes the handler for asynchronous wait operation of timer will be still executed, where the freed percentile is certainly referenced. This will lead to heap-use-after-free error.

The solution for this problem will be extend the lifetime of percentile: during the construction of the percentile, it can increment its ref count (its base class is ref_counter) since it is referenced by timer. This effectively extends the lifetime of percentile: even if both of its metric entity and registry are destructed, it can still survive in the memory for the reason that its ref count is 1 at least.

Once the percentile should be released when it will retire from metric entity or the metric registry (singleton) will be destructed, the close() method for percentile will be invoked. close() will change the state of timer from running to closing, and also call cancel() for the timer. Then, whether the timer is cancelled successfully or not, the timer will finally be stopped: if successful, the callback for cancel will be executed immediately; otherwise, the closing state will tell the timer to stop finally, albeit sometimes after a while.

@empiredan
empiredan marked this pull request as draft July 25, 2022 02:41
@github-actions github-actions Bot added the cpp label Jul 25, 2022
@empiredan
empiredan marked this pull request as ready for review July 26, 2022 15:41
@empiredan
empiredan marked this pull request as draft July 26, 2022 15:42
@empiredan
empiredan marked this pull request as ready for review July 27, 2022 07:18
@empiredan
empiredan requested a review from acelyc111 July 27, 2022 07:18
@acelyc111 acelyc111 added the type/bug-fix This PR fixes a bug. label Jul 27, 2022
Comment thread src/rdsn/include/dsn/utility/metrics.h Resolved
Comment thread src/rdsn/src/utils/metrics.cpp Resolved
@empiredan
empiredan merged commit 50d8451 into apache:master Jul 28, 2022
GehaFearless pushed a commit to GehaFearless/incubator-pegasus that referenced this pull request Aug 3, 2022
@foreverneverer foreverneverer mentioned this pull request Aug 4, 2022
ZhongChaoqiang pushed a commit to ZhongChaoqiang/incubator-pegasus that referenced this pull request Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants