Bug 2545

Could you add a regression test for this to the repo? We do have some pending
changes for the SArgument templates when the repo is open again

I'll merge the fix and the test(TAO_ROOT/tests/DII_Collocation_Tests) to DOC
repository.

could you first commit the test and a day after the regression, then the test
stats show clearly that this fails

Reassing to reporter who will take care of this

The DII_Collocated_Tests was committed and it was added to tao_orb_tests.lst so
it should show up on the test matrix. I plan commit the fix tomorrow.

Could you provide an unified diff of your change. Jeff and me have a lot of
pending changes, including to the argument handling, that way we can see if we
get conflicts in advance

I used "cvs diff -u -N" to generate the patch so you just need simply apply the
patch. A few new files are added. 

A tao/DynamicInterface/DII_Arguments_Converter_Impl.cpp
A tao/DynamicInterface/DII_Arguments_Converter_Impl.h
A tao/PortableServer/DII_Arguments_Converter.h

Created attachment 552 [details]
DII collocation request fix

FYI, I added a new test to verify if DII collocated twoway request invocation
via invoke() works. It appears work. The DII_Collocation_Tests now has two test
cases.

$TAO_ROOT/tests/DII_Collocation_Tests/oneway
$TAO_ROOT/tests/DII_Collocation_Tests/twoway

Thanks for the diff, seems there are no huge conflicts. Please use true/false
for boolean instead of 1/0. 

Why did you revert the const in tao/TAO_Server_Request.h? 

Then about the change in get_arg, so far as I can tell AMI in collocation gives
exactly the same problems, see  bug 2503. Would a dynamic cast solve the issue
for DII also, see bug 2503?

We do have a lot of changes to this file because of the cleanup/refactoring.
Could you maybe wait with your changes until all refactoring is in and maybe
retest things with a dynamic cast in get_arg? 

Have you been able to verify if then interceptors are also working fine with DII?

added depends

Reply to Johnny's comments.

>> Thanks for the diff, seems there are no huge conflicts. Please use true/false
>> for boolean instead of 1/0. 

I'll fix it.

>> Why did you revert the const in tao/TAO_Server_Request.h? 

The number of arguments and the pointer of argument list need be reset after  we
expand the NVList_Arguments to the list of arguments in Upcall_Wrapper::upcall ().


>> Then about the change in get_arg, so far as I can tell AMI in collocation gives
>> exactly the same problems, see  bug 2503. Would a dynamic cast solve the issue
>> for DII also, see bug 2503?

 
The get_*_arg () issue is similar to bug 2503, but the DII request arguments in
NVList still needs be expanded to list of arguments.

I tried dynamic_cast approach and the test got compilation errors. Here is my
change for get_in_arg ().

    template<typename T, typename IN_ARG_TYPE>
    IN_ARG_TYPE
    get_in_arg (TAO_Operation_Details const * details,
                TAO::Argument * const * skel_args,
                size_t i)
    {
      IN_ARG_TYPE in_arg = dynamic_cast<typename TAO::Arg_Traits<T>::in_arg_val *> (
            details->args ()[i])->arg ();
      if (in_arg == 0)
      {
        in_arg = dynamic_cast<typename TAO::SArg_Traits<T>::in_arg_val *> (
            skel_args[i])->arg ();
      }
      return in_arg;
}
 

The test got the compilation errors because some IN_ARG_TYPE has no operator==
defined. e.g.

TestS.cpp
..\..\..\tao\PortableServer\get_arg.h(85) : error C2678: binary '==' : no
operator found which takes a left-hand operand of type 'const
Test::Bounded_Var_Size_Arg' (or there is no acceptable conversion)
        TestS.cpp(1794) : see reference to function template instantiation
'IN_ARG_TYPE
TAO::Portable_Server::get_in_arg<Test::Bounded_Var_Size_Arg,TAO::Var_Size_SArg_Traits_T<T,T_var,T_out,Insert_Policy>::in_arg_type>(const
TAO_Operation_Details *,TAO::Argument *const * ,size_t)' being compiled
        with
        [
           
IN_ARG_TYPE=TAO::Var_Size_SArg_Traits_T<Test::Bounded_Var_Size_Arg,Test::Bounded_Var_Size_Arg_var,Test::Bounded_Var_Size_Arg_out,TAO::Any_Insert_Policy_Stream<Test::Bounded_Var_Size_Arg>>::in_arg_type,
            T=Test::Bounded_Var_Size_Arg,
            T_var=Test::Bounded_Var_Size_Arg_var,
            T_out=Test::Bounded_Var_Size_Arg_out,
            Insert_Policy=TAO::Any_Insert_Policy_Stream<Test::Bounded_Var_Size_Arg>
        ]


Now I realized the patch I provided did not fix the twoway requests since the
twoway test just tested IN arguments and did not test OUT, INOUT and RETURN
arguments. I think it should be fixed as this get_*_arg issue been resolved.

Do you have any plan in fixing the get_*_arg issue in bug 2503 ?


>> We do have a lot of changes to this file because of the cleanup/refactoring.
>> Could you maybe wait with your changes until all refactoring is in and maybe
>> retest things with a dynamic cast in get_arg? 

No problem. We also wait for customer's funding for this fix merging to DOC
repository. When will you be done with the cleanup/refactorings work ? 


>> Have you been able to verify if then interceptors are also working fine with DII?

No.

Could you extend the twoway test with inout, out and retn arguments? The
refactoring is committed as soon as the repo and teststats are stable and we get
permissions to commit. We are thinking how to fix 2503 but have not found time
to have another look. Instead of IN_ARG_TYPE try "typename
TAO::Arg_Traits<T>::in_arg_val *" Can you make sure the regression tests compile
fine on cvs head, they now fail.

Tried use "typename
TAO::Arg_Traits<T>::in_arg_val *" instead of IN_ARG_TYPE and got compilation
errors. 

PolicyS.cpp
..\tao\PortableServer\get_arg.h(85) : error C2440: 'initializing' : cannot
convert from 'const CORBA::Char *' to
'TAO::UB_String_Arg_Traits_T<T,T_var,T_out,Insert_Policy>::in_arg_val *'
        with
        [
            T=CORBA::Char,
            T_var=CORBA::String_var,
            T_out=CORBA::String_out,
            Insert_Policy=TAO::Any_Insert_Policy_AnyTypeCode_Adapter<const
CORBA::Char *>
        ]
        Types pointed to are unrelated; conversion requires reinterpret_cast,
C-style cast or function-style cast



I'll fix the compilation errors in DII_Collocation_Tests and will extend the
twoway test.

I'll remove the twoway test since our customer only fund the fix for the oneway
DII collocation request fix.

Since the committing will be hold until we get the PO from the customer, I'll
also remove the oneway test from the tao_orb_test.lst so it will not show red on
scoreboard.

To my idea in tests we shouldn't include
tao/DynamicInterface/DII_Arguments_Converter_Impl.h at all, end users include
tao/DynamicInterface/Dynamic_Adapter_Impl.h and nothing more. That should do the
trick for DII in collocated also

Could you please re add the twoway test. Just don't add it to the
tao_orb_tests.lst .This test at least can be helpfull to others in the future. 

Could it maybe be possible in someway that the portableserver library doesn't
know a thing about the DII Argument converter, so that everything is resolved in
the DII lib itself?

The tao/DynamicInterface/DII_Arguments_Converter_Impl.h include is needed to
make the test work on static builds. This include makes the static builds to
initialize DII_Arguments_Converter service object so the
Upcall_Wrapper::upcall() can find a valid DII_Arguments_Converter service object
to do the NVList_Arguments conversion.

The oneway and twoway tests were removed from tao_orb_tests.lst and I will add
the twoway test today.

We did not find a way to do the NVList_Arguments conversion on the client side
(on the DII lib). To convert on server side(in Upcall_Wrapper::upcall()) is the
best solution we found. 

I don't see why users should in a static build add another include, they have
already tao/DynamicInterface/Dynamic_Adapter_Impl.h and nothing more should be
added.

I was thinking the tao/DynamicInterface/Dynamic_Adapter_Impl.h file as in the
portableserver lib. Yes, including the
tao/DynamicInterface/Dynamic_Adapter_Impl.h should also fix the test on static
builds. You said the tao/DynamicInterface/Dynamic_Adapter_Impl.h is already
included, but I can not see in the test that has the include. Am I missing
something ? 

The oneway DII fix will be merged in after we get the PO from the customer.

I assume then all remarks I had will be addressed? Btw, don't checkin any change
before the changes of Jeff and me are in the repo, else we get conflicts.

especially the get_arg changes need to be revisited

We got the PO from the customer. Now we can continue this fix. Was the changes
from Johnney and Jeff are in the repo ?

code is in the repo but not the full set of changes for 2503, that is not fixed
yet. You can't commit until the repo is fully stable again, watch devo. Please
provide full patches for review before checkin

Created attachment 559 [details]
DII collocated oneway fix

Johnny,
  The test DII_Collocation_Tests/oneway was broken a week ago due to the
unstable TAO version. I was busy working on other tasks after that, now I got
cycle to  complete this merge. I tested it today and it passed. The patch was
attached. Could you examine the patch and let me know if it's a good time to
commit this fix ?


Thanks,
Yan

My remarks, Invocation_Adapter.h, comment say 1, should say true, make local
variables that are set but not changed after that const, for example the sz
variable. The change in get_arg.h is something I still don't like, see bug 2503
for more information on this, this should be handled more generic, there
shouldn't be more checks in these methods which are in the critical path. 

Created attachment 561 [details]
updated patch of DII collocation oneway fix

A new patch was attached. 
The new patch has no changes to get_arg.h. The NVList argument is expanded  to
*SArguments that is the skel_args passed to the get_in_arg (), I just need reset
the args_ in the operation details object to 0 so the get_in_arg() can get the
arguments from skel_args instead of from arguments in operation details.


Let me know if there are any problems and when I can commit these changes in.

Instead of clearing get_arg, see bug 2503, we should add a flag to operation
details to indicate which set of arguments should be used. This flag then can be
checked in get_arg.h in all methods and can be changed by your derived class
easily and we can then also fix the AMI problems at the same time using the same
flag.

Now 2503 has been fixed this one could be fixed also

I have merged the fix, but have not got time to test. It will be committed in
today or tomorrow. 

could you also enable the compilation and test of the twoway DII test you added?

The current twoway test was not fully implemented. It based on the oneway test
and  it used the similar idl interface and just changed oneway to twoway method
so it actually just tested "IN" arguments. I think we need add more operations
with other arguments (e.g. "OUT", "INOUT", "RET").

I was busy at this moment. If you need me to do that soon, please send a request
to our RT system so our management can know the status and assign it to me.

Fri Aug 18 17:00:15 UTC 2006  Yan Dai  <dai_y@ociweb.com>

        Merged OCI changes that fix the problem that CORBA::Request::send_oneway()
        crashes on get_in_arg() or gives incorrect arguments when the request is
        collocated oneway request. These fixes are combination of 
        "Tue Dec 27 13:20:58 USMST 2005  Yan Dai  <dai_y@ociweb.com>" and 
        "Tue May  2 16:52:43 UTC 2006  Chad Elliott  <elliott_c@ociweb.com>"
        (See BugZilla #2545 for details). 
        Note this only fixed the oneway DII collocation requests, the twoway 
        collocation request was not tested yet.
              
        * tao/Invocation_Adapter.cpp:
        * tao/Invocation_Adapter.h:
        * tao/Invocation_Adapter.inl:
        
          Added is_dii_request_ data member.
          Added an extra parameter is_dii_request(defaults to false - not a dii
request)
          to the invoke() function so it can be passed to the Operation_Details 
          to mark the request is a dii request.

        * tao/operation_details.h:
        * tao/operation_details.i:
        
          Added is_dii_request_ data member and the accessor and added the
is_dii_request
          parameter to the constructor.

        * tao/DynamicInterface/DII_Arguments.h:
        * tao/DynamicInterface/DII_Arguments.inl:
        
          Added accessor to the NVList.
          
        * tao/PortableServer/Upcall_Wrapper.cpp:

          Updated upcall () to use the DII_Argument_Convert to expand the 
          DII request parameter from NVList to list of *SArgument.
          This would make DII request parameters in NVList from the client
          side to be changed to the list of arguments so the server side 
          can correctly retrieve the arguments.

        * tao/DynamicInterface/DII_Arguments_Converter_Impl.cpp:
        * tao/DynamicInterface/DII_Arguments_Converter_Impl.h:
        * tao/PortableServer/DII_Arguments_Converter.h:
        
          An abstract class DII_Arguments_Converter is added for conversion 
          of the NVList to list of *SArgument.        
          These new files are added to resolve the library circuit dependency
          problem. This makes the conversion of NVList to list of *SArgument
          can be done in DynamicInterface instead of in PortableServer. The 
          expanded skel args are used in get_in_arg() to give the correct
          "IN" arguments. 
          
        * tao/DynamicInterface/DII_Invocation_Adapter.cpp:
        
          Passed is_dii_request true to Invocation_Adapter constructor to 
          indicate it's a dii request invocation.

        * tao/DynamicInterface/Request.cpp:
                
          Passed is_dii_request true to construct Invocation_Adapter object to
          indicate it's a dii request invocation.
          
        * tests/DII_Collocation_Tests/oneway/Collocated_Test.cpp:
        
          Removed the commented include.

After merging the changes and enabling the test by Yan the test stats do show
that the test is failing in some builds

The TAO/tests/DII_Collocation_Tests/oneway/run_test.pl passed on the nightly
builds. This showed the problem was fixed. There are more efforts need be done
for the twoway collocation DII requests - add more test cases to twoway test to
verify if twoway collocation DII requests were also fixed by this oneway fix.
Since this bug just cover oneway fix and it was done, I'll change the status to
be RESOLVED.