Bug 1495 - Use effective profile in colocation decisions
Summary: Use effective profile in colocation decisions
Status: RESOLVED FIXED
Alias: None
Product: TAO
Classification: Unclassified
Component: ORB (show other bugs)
Version: 1.3.1
Hardware: All All
: P3 normal
Assignee: Johnny Willemsen
URL:
Depends on:
Blocks: 133 2289
  Show dependency tree
 
Reported: 2003-04-21 11:23 CDT by Carlos O'Ryan
Modified: 2006-02-23 04:42 CST (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos O'Ryan 2003-04-21 11:23:15 CDT
When an object reference is forwarded the ORB fails to use the new location in
its colocation decisions.  The fix should be relatively simple,
TAO_ORB_Core::collocation_strategy() needs to be modified so the effective
object reference is used, currently it uses the original object reference.
Comment 1 Carlos O'Ryan 2003-04-21 11:24:28 CDT
Bug 133 requires two separate fixes, the current bug (1495) solves the most
common problem.
Comment 2 Nanbor Wang 2003-05-07 22:53:56 CDT
Accepted for tao-support
Comment 3 Johnny Willemsen 2004-04-02 07:14:07 CST
Added some comment about a bug found. Will be fixed after x.4.1 is released, 
found while looking at this bug.

From Collocated_Invocation I have the following constructor, notice that the 
first argument is the effective target, the second is the target.

    /// Constructor used by TAO::Invocation_Adapter
    /**
     * @param et, The effective target in which this invocation is now
     * flowing
     *
     * @param t, The target on which invocation first started.
     *
     * @param stub, Stub for target @t
     *
     * @param detail, operation details of the invocation including
     * the service context list
     *
     * @param response_expected, flag to indicate whether the
     * operation encapsulated by @a detail returns a response or not.
     */
    Collocated_Invocation (CORBA::Object_ptr et,
                           CORBA::Object_ptr t,
                           TAO_Stub *stub,
                           TAO_Operation_Details &detail,
                           bool response_expected = true);

But now from Invocation_Base. See the constructor below, this is called by the 
constructor of Collocated_Invocation, see here that the real target and 
effective target are switched, so first target, then effective target.

    /**
     * @param otarget, The original target on which this invocation
     * was started.
     *
     * @param target, the target on which this invocation is flowing
     * ie. the effective target
     *
     * @param op, operation details of the invocation on @a target
     *
     * @param response_Expected, flag to indicate whether the
     * operation encapsulated by @a op returns a response or not.
     */
    Invocation_Base (CORBA::Object_ptr otarget,
                     CORBA::Object_ptr target,
                     TAO_Stub *stub,
                     TAO_Operation_Details &op,
                     bool response_expected);

The implementation of the Collocated_Invocation is as below. The base 
constructor is called with the same argument order, so no switching.

  Collocated_Invocation::Collocated_Invocation (CORBA::Object_ptr et,
                                                CORBA::Object_ptr t,
                                                TAO_Stub *stub,
                                                TAO_Operation_Details &detail,
                                                bool response_expected)
    : Invocation_Base (et,
                       t,
                       stub,
                       detail,
                       response_expected)

When I now look at the constructor of Invocation_Base I see that the effective 
target is passed as t and this is stored in target_. But from 
Collocated_Invocation side we pass the effective target as first argument! 

  Invocation_Base::Invocation_Base (CORBA::Object_ptr ot,
                                    CORBA::Object_ptr t,
                                    TAO_Stub *stub,
                                    TAO_Operation_Details &details,
                                    bool response_expected)
    : details_ (details)
    , forwarded_to_ (0)
    , response_expected_ (response_expected)
    , otarget_ (ot)
    , target_ (t)
    , orb_core_ (stub->orb_core ())

The method Invocation_Base::effective_target() method returns target_ but 
because the arguments are swapped not the effective target is returned but the 
original target.

From Invocation_Adapter.cpp is the following code:

            Collocated_Invocation coll_inv (effective_target,
                                            this->target_,
                                            stub,
                                            op,
                                            this->type_ == 
TAO_TWOWAY_INVOCATION);

So, the first argument is effective_target, this will be passed to 
Invocation_Base as original target, and stored in otarget_. The second argument 
is the original target, this is passed to t and stored in target_.

So, the result is that the variables effective_target and target_ are stored 
swapped.

When now in Collocated_Invocation::invoke the code below is executed (some 
lines removed for clearness), the dispatch is done on the original target 
instead of the effective target.

  Invocation_Status
  Collocated_Invocation::invoke (Collocation_Proxy_Broker *cpb,
                                 Collocation_Strategy strat
                                 ACE_ENV_ARG_DECL)
    ACE_THROW_SPEC ((CORBA::Exception))
  {
    Invocation_Status s = TAO_INVOKE_FAILURE;

    ACE_TRY
      {
        cpb->dispatch (this->effective_target (),
                       this->forwarded_to_.out (),
                       this->details_.args (),
                       this->details_.args_num (),
                       this->details_.opname (),
                       this->details_.opname_len (),
                       strat
                       ACE_ENV_ARG_PARAMETER);
        ACE_TRY_CHECK;

Checked this with Bala. We will change the order of the arguments of 
Collocation_Invocation and swap there then the arguments to make things 
consistent.
Comment 4 Johnny Willemsen 2004-04-02 07:14:57 CST
this one is for me
Comment 5 Johnny Willemsen 2004-04-06 03:02:22 CDT
For always creating a collocated proxy broker the following changes in the IDL 
compiler as proposal (removed collocated argument, check pointer before using 
it)

Index: be/be_interface.cpp
===================================================================
RCS file: /project/cvs-repository/ACE_wrappers-
repository/TAO/TAO_IDL/be/be_interface.cpp,v
retrieving revision 1.195
diff -u -u -r1.195 be_interface.cpp
--- be/be_interface.cpp	26 Mar 2004 04:42:03 -0000	1.195
+++ be/be_interface.cpp	6 Apr 2004 07:29:31 -0000
@@ -580,11 +580,11 @@
 
       *os << be_nl << "{" << be_idt_nl
           << "this->" << this->flat_name ()
-          << "_setup_collocation (_tao_collocated);";
+          << "_setup_collocation ();";
 
       if (this->is_abstract ())
         {
-          *os << be_nl 
+          *os << be_nl
               << "ACE_UNUSED_ARG (oc);";
         }
 
@@ -645,7 +645,7 @@
 
 // =================================================================
 
-class TAO_IDL_Gen_OpTable_Worker 
+class TAO_IDL_Gen_OpTable_Worker
   : public TAO_IDL_Inheritance_Hierarchy_Worker
 {
 public:
@@ -681,7 +681,7 @@
 
 // =================================================================
 
-class Pure_Virtual_Regenerator 
+class Pure_Virtual_Regenerator
   : public TAO_IDL_Inheritance_Hierarchy_Worker
 {
 public:
Index: be/be_visitor_interface/interface_ch.cpp
===================================================================
RCS file: /project/cvs-repository/ACE_wrappers-
repository/TAO/TAO_IDL/be/be_visitor_interface/interface_ch.cpp,v
retrieving revision 1.71
diff -u -u -r1.71 interface_ch.cpp
--- be/be_visitor_interface/interface_ch.cpp	16 Mar 2004 15:53:16 -0000
	1.71
+++ be/be_visitor_interface/interface_ch.cpp	6 Apr 2004 07:15:58 -0000
@@ -191,7 +191,7 @@
                          "codegen for scope failed\n"),
                         -1);
     }
-    
+
   if (node->is_local ())
     {
       if (node->convert_parent_ops (this) == -1)
@@ -253,7 +253,7 @@
       if (! node->is_abstract ())
         {
           *os << "// Concrete interface only." << be_nl
-              << node->local_name () << " (int collocated = 0);"
+              << node->local_name () << " (void);"
               << be_nl << be_nl;
         }
 
@@ -262,7 +262,7 @@
           << "// parents piece of the given class in the right mode."
           << be_nl
           << "virtual void " << node->flat_name ()
-          << "_setup_collocation (int collocated);" << be_nl << be_nl;
+          << "_setup_collocation (void);" << be_nl << be_nl;
     }
 
   if (node->is_abstract () || node->is_local ())
Index: be/be_visitor_interface/interface_cs.cpp
===================================================================
RCS file: /project/cvs-repository/ACE_wrappers-
repository/TAO/TAO_IDL/be/be_visitor_interface/interface_cs.cpp,v
retrieving revision 1.99
diff -u -u -r1.99 interface_cs.cpp
--- be/be_visitor_interface/interface_cs.cpp	29 Jan 2004 18:04:07 -0000
	1.99
+++ be/be_visitor_interface/interface_cs.cpp	6 Apr 2004 07:34:05 -0000
@@ -165,11 +165,11 @@
     {
       *os << be_nl << be_nl
           << node->name () << "::" << node->local_name ()
-          << " (int collocated)" << be_nl
+          << " (void)" << be_nl
           << " : the" << node->base_proxy_broker_name () << "_ (0)" << be_nl
           << "{" << be_idt_nl
           << "this->" << node->flat_name ()
-          << "_setup_collocation (collocated);" << be_uidt_nl
+          << "_setup_collocation ();" << be_uidt_nl
           << be_uidt << "}";
     }
 
@@ -178,10 +178,13 @@
       *os << be_nl << be_nl
           << "void" << be_nl
           << node->name () << "::" << node->flat_name ()
-          << "_setup_collocation (int collocated)" << be_nl
+          << "_setup_collocation ()" << be_nl
           << "{" << be_idt_nl
-          << "if (collocated";
-       
+          << "if ("
+          << node->flat_client_enclosing_scope ()
+          << node->base_proxy_broker_name ()
+          << "_Factory_function_pointer";
+
        // Right now (29-01-04) we don't support collocation for
        // abstract interfaces, and the 'collocated' arg will always
        // be 0. However, just to be safe, we add a
@@ -189,12 +192,12 @@
        // (which at present is also 0 for abstract interfaces),
        // in case the logic is changed in the future.
        if (node->is_abstract ())
-        {   
-          *os << " && " << node->flat_client_enclosing_scope () 
-              << node->base_proxy_broker_name () 
+        {
+          *os << " && " << node->flat_client_enclosing_scope ()
+              << node->base_proxy_broker_name ()
               << "_Factory_function_pointer";
         }
-          
+
       *os << ")" << be_idt_nl
           << "{" << be_idt_nl
           << "this->the" << node->base_proxy_broker_name ()
Index: be/be_visitor_operation/operation.cpp
===================================================================
RCS file: /project/cvs-repository/ACE_wrappers-
repository/TAO/TAO_IDL/be/be_visitor_operation/operation.cpp,v
retrieving revision 1.50
diff -u -u -r1.50 operation.cpp
--- be/be_visitor_operation/operation.cpp	11 Feb 2004 19:13:15 -0000
	1.50
+++ be/be_visitor_operation/operation.cpp	6 Apr 2004 07:32:06 -0000
@@ -429,11 +429,8 @@
           << "if (this->the" << intf->base_proxy_broker_name () << "_ == 0)"
           << be_idt_nl
           << "{" << be_idt_nl
-          << intf->flat_name () << "_setup_collocation ("
-          << be_idt << be_idt_nl
-          << "this->ACE_NESTED_CLASS (CORBA, Object)::_is_collocated ()"
+          << intf->flat_name () << "_setup_collocation ();"
           << be_uidt_nl
-          << ");" << be_uidt << be_uidt_nl
           << "}" << be_uidt_nl << be_nl;
     }
 
@@ -648,7 +645,7 @@
 {
   AST_Argument *arg = 0;
   UTL_ScopeActiveIterator arg_decl_iter (node, UTL_Scope::IK_decls);
-  
+
   if (ami)
     {
       // Skip the reply handler (first argument).
@@ -693,12 +690,12 @@
                                                    TAO_OutStream *os)
 {
   AST_Typedef *alias = 0;
-  
+
   if (bt->node_type () == AST_Decl::NT_typedef)
     {
       alias = AST_Typedef::narrow_from_decl (bt);
     }
-    
+
   AST_Decl::NodeType nt = bt->unaliased_type ()->node_type ();
 
   if (nt == AST_Decl::NT_string || nt == AST_Decl::NT_wstring)

Comment 6 Johnny Willemsen 2004-04-22 03:47:22 CDT
Changes are in the repo. Regression test is also added and enabled. Now wait 
for all build results before closing this one
Comment 7 Johnny Willemsen 2004-06-18 05:42:38 CDT
This one is fixed.
Comment 8 Johnny Willemsen 2005-11-01 03:49:34 CST
reopening, doesn't seem fully fixed. Added blocks
Comment 9 Simon McQueen 2006-02-23 04:42:55 CST
Fixed - Tue Feb 21 17:48:24 UTC 2006  Simon McQueen  <sm@prismtech.com>.
Regression test is now passing.