Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed dangling pointer in Balancer #603

Merged
merged 3 commits into from
Jul 10, 2019
Merged

Fixed dangling pointer in Balancer #603

merged 3 commits into from
Jul 10, 2019

Conversation

dutor
Copy link
Contributor

@dutor dutor commented Jul 10, 2019

As title.

@dutor dutor added the ready-for-testing PR: ready for the CI test label Jul 10, 2019
@nebula-community-bot
Copy link
Member

Unit testing passed.

sherman-the-tank
sherman-the-tank previously approved these changes Jul 10, 2019
View reviewed changes
Copy link
Member

@sherman-the-tank sherman-the-tank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!! 👍

I think we should enable ASAN by default during development

@dangleptr
Copy link
Contributor

dangleptr commented Jul 10, 2019 •
edited
Loading

As original implementation, after plan_.reset(), we did not access any members in plan_ in onFinished_.

The program crashed?

@dutor
Copy link
Contributor Author

dutor commented Jul 10, 2019

As original implementation, after plan_.reset(), we did not access any members in plan_ in onFinished_.

The program crashed?

After plan_ got released, onFinished_ also no longer persists, as well as all data it has captured, this in this case. So running_ cannot be reached any more, even though it's still alive.

@dangleptr
Copy link
Contributor

As original implementation, after plan_.reset(), we did not access any members in plan_ in onFinished_.
The program crashed?

After plan_ got released, onFinished_ also no longer persists, as well as all data it has captured, this in this case. So running_ cannot be reached any more, even though it's still alive.

Fair enough!

dangleptr
dangleptr previously approved these changes Jul 10, 2019
View reviewed changes
@dutor dutor dismissed stale reviews from dangleptr and sherman-the-tank via e47d62b July 10, 2019 05:47
@dutor
Copy link
Contributor Author

dutor commented Jul 10, 2019

Thanks to @wadeliuyi for discovering the race condition problem.

sherman-the-tank
sherman-the-tank approved these changes Jul 10, 2019
View reviewed changes
Copy link
Member

@sherman-the-tank sherman-the-tank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! 👍

dangleptr
dangleptr approved these changes Jul 10, 2019
View reviewed changes
Copy link
Contributor

@dangleptr dangleptr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!

@nebula-community-bot
Copy link
Member

Unit testing passed.

@nebula-community-bot
Copy link
Member

Unit testing passed.

@dutor dutor merged commit 0d70d53 into vesoft-inc:master Jul 10, 2019
@dutor dutor deleted the fix-dangling-ptr branch July 10, 2019 06:16
tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021
@dutor
Fixed dangling pointer in Balancer (vesoft-inc#603)
1fbe357 
* Fixed dangling pointer in Balancer

* Put CAS at the bottom to eliminate the race condition
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Mar 21, 2022
@Shylock-Hg @CPWstatic
Push limit to Traverse. (vesoft-inc#603)
447fab4 
* Push limit to Traverse.

* Fix case.

* Fix format.

* Add int vid cases.

* Support parameter.

* Format.

Co-authored-by: cpw <13495049+CPWstatic@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherman-the-tank sherman-the-tank sherman-the-tank approved these changes

@dangleptr dangleptr dangleptr approved these changes

@monadbobo monadbobo Awaiting requested review from monadbobo

@CPWstatic CPWstatic Awaiting requested review from CPWstatic

@laura-ding laura-ding Awaiting requested review from laura-ding

Assignees
No one assigned
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dutor @nebula-community-bot @dangleptr @sherman-the-tank