-
Notifications
You must be signed in to change notification settings - Fork 242
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
Fixes an init race condition #93
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch. I could see that this is a potential race condition.
I am requesting changes though because I would like to have some tests for it. It would be great if you could write a set of unit tests to cover the new functionality.
rosbag2/include/rosbag2/writer.hpp
Outdated
@@ -77,6 +77,16 @@ class ROSBAG2_PUBLIC Writer | |||
*/ | |||
virtual void create_topic(const TopicMetadata & topic_with_type); | |||
|
|||
/** | |||
* Remove a new topic in the underlying storage. If creation of subscription fails remove the, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please format for one sentence per line.
* Remove a new topic in the underlying storage. If creation of subscription fails remove the, | |
* Remove a new topic in the underlying storage. | |
* If creation of subscription fails remove, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Karsten1987 most of the files have similar comments. Would you want me to edit them for all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's fine if you only tackle the comments of the functions you're modifying. Otherwise the PR is getting too big.
rosbag2/include/rosbag2/writer.hpp
Outdated
* \param topic_with_type name and type identifier of topic to be created | ||
* \throws runtime_error if the Writer is not open. | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -106,13 +106,22 @@ void Recorder::subscribe_topics( | |||
|
|||
void Recorder::subscribe_topic(const rosbag2::TopicMetadata & topic) | |||
{ | |||
if(writer_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check shouldn't be necessary because writer_
stays a initialized shared pointer.
@@ -106,13 +106,22 @@ void Recorder::subscribe_topics( | |||
|
|||
void Recorder::subscribe_topic(const rosbag2::TopicMetadata & topic) | |||
{ | |||
if(writer_) { | |||
writer_->create_topic(topic); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it be sufficient to call writer_->create_topic(topic)
before we call subscriptions_.push_back(subscriptions)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. But, don't you feel we may need a fallback when subscription creation fails and the db still has an entry for the same. At the same time, when the subscription is started again, the race would still exist. So may be we may have to clean up the db, so added the remove api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your last point.
Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
Hi @Karsten1987 I've updated the patch. Please review. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch looks better now. I would still like to have it tested if possible.
Hi @Karsten1987 I've updated the patch with a test case for remove_topic at the lower most layer of the architecture. Once this API starts being used extensively we may have to add more test cases into other layers of the code. Please let me know if this approach is ok. I'm trying to keep the patchset as minimal as possible. So that I could build on this in future. |
Hello @Karsten just reminding you about this pr |
thanks for the patch. It looks good to me. However, in order to run CI on it, you have to rebase your branch to the latest master. |
Hi Karsten, I've rebased to master. Let me know if anyting else. |
CI: looks like the tests are not completely modified for this:
|
Hi Karsten, Had add the new remove API to other dependent test cases. Fixed them now. Please have a look. |
@Karsten1987 |
@sriramster Sorry for being silent on this. Do you mind rebasing this branch to the latest master? |
@Karsten1987 Hi Karsten, done. Merged the changes. Please let me know if anything else needs to be done. Sorry for the delay. I missed your comment somehow. |
@sriramster I am sorry, I might have to ask you one more time to rebase. We've had to fix the master build first. Once rebased, I am happy to run CI on your branch. |
Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
Looks like something went wrong here while rebasing. |
Hi Karsten, Do you've the log? of what went wrong? I'm unable to follow it through the tree. Could you help? |
Your branch has now 24 commits where 19 of them are already on master. So you have to rebase correctly to only have your 6 commits on this branch. Does this make sense? |
Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
@Karsten1987 Could you review this now? Hope there are no issues. Gradually the branch is becoming unclean for me. :-/ |
I think something didn't quite work out during your rebasing. Make sure you end up with only the commits you've actually authored. So for example, right now there are 33 commits on this branch - whereas I believe only 6 commits are actually yours.
There might be some conflicts you have to resolve locally on your side, but this should help you end up with a branch which only has your 6 commits on top of the upstream master branch. |
…subscriber in the API, and the subscriber has the data already available in the callback (Cause of existing publishers) the db entry for the particular topic would not be availalble, which in turn returns an SqliteException. This is cause write_->create_topic() call is where we add the db entry for a particular topic. And, this leads to crashing before any recording. Locally I solved it by adding the db entry first, and if create_subscription fails, remove the topic entry from the db and also erase the subscription. Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
Well. I used the same technique to rebase. Let me know if this works. I've cleaned up and pushed again. |
So I think we have to start from a clean sheet here. There are now 49 commits on this branch and most of them are duplicated. Here is what I propose:
This will reset your branch to the latest upstream branch. Then with this cherry-pick command you can only apply the commit range you've contributed. You might have to resolve conflicts along the way of cherry-picking. Alternatively, you can close this PR, check-out a new branch (I recommend renaming it to something else than master, e.g. If you then look at your |
I fixed my repo by force pushing it. Somehow not used to this model, and taking some time to understand it. Please close the pull request if this does'nt work. I'll re-do this work and raise a new request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Thanks for iterating over it.
Thanks @Karsten1987 I'm not used to rebase workflow, I follow fetch, review, merge and then push. |
This could probably be a race condition,
For ex: When we've create a subscriber in the API, and the subscriber has the data already available in the callback (Cause of existing publishers) the db entry for the particular topic would not be availalble, which in turn returns an SqliteException. This is cause write_->create_topic() call is where we add the db entry for a particular topic. And, this leads to crashing before any recording. Locally I solved it by adding the db entry first, and if create_subscription fails, remove the topic entry from the db and also erase the subscription.