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

Move event subscribing ahead of service advertising. Fix #82. #83

Merged
merged 1 commit into from
Aug 19, 2015
Merged

Move event subscribing ahead of service advertising. Fix #82. #83

merged 1 commit into from
Aug 19, 2015

Conversation

patrickcjh
Copy link
Contributor

No description provided.

@wjwwood
Copy link
Member

wjwwood commented Aug 17, 2015

Can you explain how you think this addresses #82?

@patrickcjh
Copy link
Contributor Author

After the capability provider is started by the launch manager, the launch manager will publish a CapabilityEvent message containing the provider name, process ID (PID) and event type.

If the capability server starts listening to the CapabilityEvent messages at an earlier time, it will not miss any of these CapabilityEvent messages, and therefore can register the PID and "launched" status of the provider.

@wjwwood
Copy link
Member

wjwwood commented Aug 19, 2015

Ok, I think you're right that this might improve the situation, but I believe there is still a race condition. I still think that the "correct" thing to do is to depend on, and then call a service which is the final answer on whether or not the capability server is ready.

The tests pass and the coverage is still 99%, so I'll go a head and merge this, but I'd still like to see the service as described in #82.

wjwwood added a commit that referenced this pull request Aug 19, 2015
Move event subscribing ahead of service advertising. Fix #82.
@wjwwood wjwwood merged commit d516436 into osrf:master Aug 19, 2015
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.

2 participants