Improve transport::Publisher reliability

[nlamprian]

It has been observed that OnPublishComplete can run before result is checked in SendMessage. In that case, the pubIds entry is deleted by OnPublishComplete, and then it's recreated in SendMessage. This blocks publication of any future messages, thus rendering the publisher unusable.

The fix guards access to pubIds in SendMessage, and it processes the pubIds entry only if it still exists.

[chapulina]

Thanks for the PR, @nlamprian . Do you think it would be possible to write a test that exercises the fix?

[nlamprian]

Oof, this will require some effort. I'll need to familiarize myself with the full implementation to understand how to trigger the conditions. I'll give it a try.

[nlamprian]

The unit test is added. It successfully reproduces the error. Basically, for the error to show up, there have to be callbacks in the Publication. A callback is added by a subscription to the Publication, which, in this case, happens in the ConnectionManager with a SubscriptionTransport. It's still not clear to me how everything around all of this works, but nonetheless, I replicated the conditions in the test. There, it can be seen that the first message is sent and received, but the second one is not.

[nlamprian]

May I please ask for an update on this? Is there something holding back the review? The publisher continues to be a problem for me, and I would hope to see it fixed soon.

[chapulina]

@osrf-jenkins run tests

Thank you for the contribution and the test, it looks good to me. Let's just run a round of CI to make sure everything is ok.

[chapulina]

@osrf-jenkins run tests again?

[chapulina]

@osrf-jenkins run tests

[chapulina]

The fix and test look good and CI is happy 👍