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.

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

Fixed the lock ordering between EDP and PDP [4557] #192

Closed
wants to merge 1 commit into from

Conversation

guillaumeautran
Copy link
Contributor

@guillaumeautran guillaumeautran commented Mar 10, 2018

Fixed a deadlock problem when removing writer proxies / reader proxies releasing the PDP mutex pior to going down proxies.

Also, converted some pointers to smart pointers to avoid memory leakage.

Issue: #190

@dhood
Copy link
Contributor

dhood commented Mar 20, 2018

This PR resolves the deadlocking described in #200

@dhood
Copy link
Contributor

dhood commented Mar 21, 2018

For what it's worth I have also tested the full ros2 CI using this branch and no other regressions were observed. This is not a comment on the code changes, just the resulting behaviour changes.

@dhood
Copy link
Contributor

dhood commented Mar 28, 2018

Could you let us know if this is likely to be reviewed in the next week, please? (We understand that your team is busy, we just need to do something about the ROS 2 tests that are failing from #200, and an ETA on this PR will help us decide if we should pin fastrtps or just wait). Thanks!

@JaimeMartin
Copy link
Member

Hi dhood,

yes, we will review this PR next week.

@dhood
Copy link
Contributor

dhood commented Mar 29, 2018

Thank you for the update @JaimeMartin

@richiware
Copy link
Member

@guillaumeautran Last week a fellow worker tells me he detected a segmentation fault using your PR. I've checked it right now and get it too.

1: [ RUN      ] BlackBox_PreallocMem.EndpointRediscovery
1: Writer is waiting discovery...
1: Writer discovery finished...
1: Reader is waiting discovery...
1: Reader discovery finished...
1: Writer is waiting removal...
1: Writer removal finished...
1: Writer is waiting discovery...
1/1 Test #1: BlackboxTests_PreallocMem ........***Exception: SegFault 28.27 sec

It's good your intention of use shared_ptr, but I believe it has repercutions knowing how that part is designed.
Anyway you can run our tests using ctest. Previously our CMakelists.txt has to have detected googletest libraries.

@guillaumeautran
Copy link
Contributor Author

Thanks for the feedback! Let me have a look.

@guillaumeautran
Copy link
Contributor Author

guillaumeautran commented Apr 4, 2018

Yeap, I see it now. The problem is the suicidal timer TimedEventImpl.cpp deleting itself. With shared pointers, once the last reference is cleared, the memory is deleted without delay.

Now, does that not worry you to have a class basically doing a delete on the this pointer?

@richiware richiware changed the title Fixed the lock ordering between EDP and PDP Fixed the lock ordering between EDP and PDP [4557] Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants