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

[DO NOT MERGE] RFC: corba: allow selecting dispatchers per-channel #227

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

doudou
Copy link
Contributor

@doudou doudou commented Oct 10, 2017

This PR implements the main idea I had for a while on how to separate communication domains under the CORBA transport.

By default, the policy is the same than the one being used now: there's one dispatcher per task context. However, one can set a policy's name_id to pick a different dispatcher, possibly sharing dispatchers between tasks (how dispatchers are allocated becomes a system builder's concern).

As soon as we assume that these methods must be thread safe, we
cannot guarantee that no other thread will try a Get, Acquire or
Release while another is deleting the mutex.

We could possibly have a fairly complex refcounting scheme to allow
for deletion, but I would rather just leave the singleton mutex live.
If static C++ initialization rules were not so complicated, the mutex
would probably be a global variable anyways.
@doudou
Copy link
Contributor Author

doudou commented Oct 10, 2017

At this stage, this has NOT been thoroughly tested. I've used it on a local machine, but never on production. The goal is to get comments, and possibly more testers.

@meyerj
Copy link
Member

meyerj commented Dec 13, 2017

Could you elaborate a bit on your motivation?

Clearly, it can be a solution for the issue reported in #248 and would allow fine-grained control for power users. But there might also simpler solutions which do not require the user to configure dispatchers manually, like:

  • spawn a pool of (4?) dispatchers
  • one dispatcher per remote host (as encoded in the IOR)
  • one dispatcher per connection
  • dynamically spawn new dispatcher threads if signal() is called while all dispatchers are busy

@meyerj
Copy link
Member

meyerj commented Dec 13, 2017

I would prefer to not re-use the name_id field for the purpose to select a dispatcher. The documentation in ConnPolicy.hpp:84 clearly states that the field is used as a transport-dependent identifier for the connection to "coordinate out of band transport such that they can find each other by name". The OCL deployer sets the default name_id from the connection name in a XML deployment file since orocos-toolchain/ocl@7ec9896 in the toolchain-2.9 branch and it is used to identify shared connection objects and to connect ports to existing buffer objects since #114. All those usages are consistent with the original documentation of the field in a sense, while here you would probably use the same dispatcher name_id for different connections.

Could we instead add a new field to the ConnPolicy struct or add a PropertyBag field to pass arbitrary transport-specific connection properties (with deep-copy semantics)? Such a field can also serve other use cases, like #218, and can nicely be integrated in the deployer's XML configuration based on property marshalling.

@doudou
Copy link
Contributor Author

doudou commented Dec 13, 2017

Motivation:

  • any built-in policy on the dispatchers is bound to have failure modes. Everyone seem content with the current policy so let's have it as default and let the system integrators actually choose something that actually works for their actual system. RTT does not know the topology of the connections and has no mean to guess it, something a system integration tool or person does. Moreover, load-balancing the connections properly over the threads automatically would be much harder than using the designer's brain.
  • I'm very much for putting an arbitrary key-value in the ConnPolicy for the purpose of transport-specific stuff, which would allow us to phase out transport-specific flags such as 'pull'.

@meyerj meyerj mentioned this pull request Apr 11, 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.

2 participants