Fixes an init race condition #93

sriramster · 2019-02-28T17:27:35Z

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.

Karsten1987

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.

Karsten1987 · 2019-03-25T17:56:12Z

rosbag2/include/rosbag2/writer.hpp

@@ -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, 


please format for one sentence per line.

Suggested change

* 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

Hi @Karsten1987 most of the files have similar comments. Would you want me to edit them for all?

it's fine if you only tackle the comments of the functions you're modifying. Otherwise the PR is getting too big.

Karsten1987 · 2019-03-25T17:56:28Z

rosbag2/include/rosbag2/writer.hpp

+   * \param topic_with_type name and type identifier of topic to be created
+   * \throws runtime_error if the Writer is not open.
+   */
+


Suggested change

Karsten1987 · 2019-03-25T18:02:28Z

rosbag2_transport/src/rosbag2_transport/recorder.cpp

@@ -106,13 +106,22 @@ void Recorder::subscribe_topics(

 void Recorder::subscribe_topic(const rosbag2::TopicMetadata & topic)
 {
+  if(writer_) {


this check shouldn't be necessary because writer_ stays a initialized shared pointer.

Karsten1987 · 2019-03-25T18:04:27Z

rosbag2_transport/src/rosbag2_transport/recorder.cpp

@@ -106,13 +106,22 @@ void Recorder::subscribe_topics(

 void Recorder::subscribe_topic(const rosbag2::TopicMetadata & topic)
 {
+  if(writer_) {
+    writer_->create_topic(topic);


wouldn't it be sufficient to call writer_->create_topic(topic) before we call subscriptions_.push_back(subscriptions) ?

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.

I agree with your last point.

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

sriramster · 2019-03-26T18:07:16Z

Hi @Karsten1987 I've updated the patch. Please review. Thanks

Karsten1987

The patch looks better now. I would still like to have it tested if possible.

sriramster · 2019-03-28T16:55:01Z

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.

sriramster · 2019-04-02T18:09:22Z

Hello @Karsten just reminding you about this pr

Karsten1987 · 2019-04-05T02:19:40Z

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.

sriramster · 2019-04-08T08:08:56Z

Hi Karsten,

I've rebased to master. Let me know if anyting else.

Karsten1987 · 2019-04-08T16:49:16Z

CI:

Linux
Linux-aarch64
macOS
Windows

looks like the tests are not completely modified for this:

16:37:49 /home/jenkins-agent/workspace/ci_linux/ws/install/class_loader/include/class_loader/meta_object.hpp:193:12: error: invalid new-expression of abstract class type ‘TestPlugin’
16:37:49      return new C;
16:37:49             ^~~~~
16:37:49 In file included from /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rosbag2/rosbag2_storage/test/rosbag2_storage/test_plugin.cpp:25:0:
16:37:49 /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rosbag2/rosbag2_storage/test/rosbag2_storage/test_plugin.hpp:27:7: note:   because the following virtual functions are pure within ‘TestPlugin’:
16:37:49  class TestPlugin : public rosbag2_storage::storage_interfaces::ReadWriteInterface
16:37:49        ^~~~~~~~~~
16:37:49 In file included from /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rosbag2/rosbag2_storage/include/rosbag2_storage/storage_interfaces/read_write_interface.hpp:21:0,
16:37:49                  from /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rosbag2/rosbag2_storage/test/rosbag2_storage/test_plugin.hpp:25,
16:37:49                  from /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rosbag2/rosbag2_storage/test/rosbag2_storage/test_plugin.cpp:25:
16:37:49 /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rosbag2/rosbag2_storage/include/rosbag2_storage/storage_interfaces/base_write_interface.hpp:40:16: note: 	virtual void rosbag2_storage::storage_interfaces::BaseWriteInterface::remove_topic(const rosbag2_storage::TopicMetadata&)
16:37:49    virtual void remove_topic(const TopicMetadata & topic) = 0;

sriramster · 2019-04-09T11:26:33Z

Hi Karsten,

Had add the new remove API to other dependent test cases. Fixed them now. Please have a look.

Karsten1987 · 2019-04-09T18:56:40Z

new round of CI:

Linux
Linux-aarch64
macOS
Windows

looks like there is some linter errors you might want to address.

sriramster · 2019-04-12T09:19:35Z

@Karsten1987
Fixed the linter errors. Sorry for the delay held up with things.

Karsten1987 · 2019-04-30T18:36:30Z

@sriramster Sorry for being silent on this. Do you mind rebasing this branch to the latest master?

sriramster · 2019-05-06T09:55:22Z

@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.

Karsten1987 · 2019-05-07T17:02:06Z

@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>

Karsten1987 · 2019-05-14T16:22:05Z

Looks like something went wrong here while rebasing.

sriramster · 2019-05-14T18:21:55Z

Hi Karsten, Do you've the log? of what went wrong? I'm unable to follow it through the tree. Could you help?

Karsten1987 · 2019-05-14T18:35:11Z

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>

sriramster · 2019-05-15T11:13:51Z

@Karsten1987 Could you review this now? Hope there are no issues. Gradually the branch is becoming unclean for me. :-/

Karsten1987 · 2019-05-17T00:50:43Z

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.
So just to be clear, you have to run

git checkout master
git fetch --all
git rebase origin/master

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>

sriramster · 2019-05-20T07:54:11Z

Well. I used the same technique to rebase. Let me know if this works. I've cleaned up and pushed again.
Thanks for the patience.

Karsten1987 · 2019-05-20T16:50:03Z

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:

cd <your/path/to/rosbag2>
git fetch --all
git checkout master
git reset --hard origin/master
git cherry-pick 3bf3313befbddaf2ff6a982fac501c81c65bb976..4841d7caf57a98d3b6094ab9bf4e88c2a3ab5fae

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. fix_init_race_condition) and put your commits on top of that new branch. As far as I can recall, the changes are not too big.

If you then look at your git log or gitk or similar visualization tool for git branches, you should only see a couple of commits on top of origin/master. In any case, once you've done either one I would ask you to compile and run tests on your workspace to verify your branch.

sriramster · 2019-05-20T19:36:02Z

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.

Karsten1987 · 2019-05-20T23:09:51Z

Happy to see you could fix it. The branch looks good now. Just fyi, whenever you rebase, there is no way around force pushing. So that's to be expected.

Running CI:

Linux
Linux-aarch64
macOS
Windows

Karsten1987

lgtm. Thanks for iterating over it.

sriramster · 2019-05-21T06:50:54Z

Thanks @Karsten1987 I'm not used to rebase workflow, I follow fetch, review, merge and then push.
I think that's what messed the branch up.

Relates from ros2/rosbag2#93

tfoote added the in review label Feb 28, 2019

mjcarroll assigned Karsten1987 Mar 21, 2019

Karsten1987 requested changes Mar 25, 2019

View reviewed changes

sriramster pushed a commit to sriramster/rosbag2 that referenced this pull request Mar 26, 2019

Fix comments for pull request ros2#93

0711fb5

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

Karsten1987 reviewed Mar 26, 2019

View reviewed changes

sriramster pushed a commit to sriramster/rosbag2 that referenced this pull request May 14, 2019

Fix comments for pull request ros2#93

1b79269

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

sriramster pushed a commit to sriramster/rosbag2 that referenced this pull request May 15, 2019

Fix comments for pull request ros2#93

b6f4dfa

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

sriramster pushed a commit to sriramster/rosbag2 that referenced this pull request May 15, 2019

Fix comments for pull request ros2#93

c5e38e0

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

Sriram Raghunathan added 5 commits May 20, 2019 13:19

Fix comments for pull request ros2#93

7f9ddd2

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

Added unit test case for remove_topics from db

98591e7

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

Fix unit tests failing by adding dependent test macros

8804882

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

Fixes the linter errors

c388d8e

sriramster pushed a commit to sriramster/rosbag2 that referenced this pull request May 20, 2019

Fix comments for pull request ros2#93

daa2ce6

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

sriramster pushed a commit to sriramster/rosbag2 that referenced this pull request May 20, 2019

Fix comments for pull request ros2#93

2dd0a60

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

sriramster pushed a commit to sriramster/rosbag2 that referenced this pull request May 20, 2019

Fix comments for pull request ros2#93

b04ed00

Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>

sriramster force-pushed the master branch from df902f0 to c388d8e Compare May 20, 2019 19:30

Karsten1987 approved these changes May 21, 2019

View reviewed changes

Karsten1987 merged commit dcd65a6 into ros2:master May 21, 2019

ruffsl added a commit to dledr/bbr_ros2 that referenced this pull request Jul 10, 2019

Update storage plugin child class for dashing

c4e892e

Relates from ros2/rosbag2#93

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes an init race condition #93

Fixes an init race condition #93

sriramster commented Feb 28, 2019

Karsten1987 left a comment

Karsten1987 Mar 25, 2019

sriramster Mar 26, 2019

Karsten1987 Mar 26, 2019

Karsten1987 Mar 25, 2019

Karsten1987 Mar 25, 2019

Karsten1987 Mar 25, 2019

sriramster Mar 26, 2019

Karsten1987 Mar 26, 2019

sriramster commented Mar 26, 2019

Karsten1987 left a comment

sriramster commented Mar 28, 2019

sriramster commented Apr 2, 2019

Karsten1987 commented Apr 5, 2019

sriramster commented Apr 8, 2019

Karsten1987 commented Apr 8, 2019

sriramster commented Apr 9, 2019

Karsten1987 commented Apr 9, 2019 •

edited

sriramster commented Apr 12, 2019

Karsten1987 commented Apr 30, 2019

sriramster commented May 6, 2019

Karsten1987 commented May 7, 2019

Karsten1987 commented May 14, 2019

sriramster commented May 14, 2019

Karsten1987 commented May 14, 2019

sriramster commented May 15, 2019

Karsten1987 commented May 17, 2019

sriramster commented May 20, 2019

Karsten1987 commented May 20, 2019

sriramster commented May 20, 2019

Karsten1987 commented May 20, 2019 •

edited

Karsten1987 left a comment

sriramster commented May 21, 2019

	* 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

Fixes an init race condition #93

Fixes an init race condition #93

Conversation

sriramster commented Feb 28, 2019

Karsten1987 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

Choose a reason for hiding this comment

sriramster commented Mar 26, 2019

Karsten1987 left a comment

Choose a reason for hiding this comment

sriramster commented Mar 28, 2019

sriramster commented Apr 2, 2019

Karsten1987 commented Apr 5, 2019

sriramster commented Apr 8, 2019

Karsten1987 commented Apr 8, 2019

sriramster commented Apr 9, 2019

Karsten1987 commented Apr 9, 2019 • edited

sriramster commented Apr 12, 2019

Karsten1987 commented Apr 30, 2019

sriramster commented May 6, 2019

Karsten1987 commented May 7, 2019

Karsten1987 commented May 14, 2019

sriramster commented May 14, 2019

Karsten1987 commented May 14, 2019

sriramster commented May 15, 2019

Karsten1987 commented May 17, 2019

sriramster commented May 20, 2019

Karsten1987 commented May 20, 2019

sriramster commented May 20, 2019

Karsten1987 commented May 20, 2019 • edited

Karsten1987 left a comment

Choose a reason for hiding this comment

sriramster commented May 21, 2019

Karsten1987 commented Apr 9, 2019 •

edited

Karsten1987 commented May 20, 2019 •

edited