>During profile selection it is required that the endpoint selector 
> calls back on the profile transport resolver ::try_connect method 
> with the profile that is going to be used. If the connect 
> is successful this sets the transport on the resolver for later use.

> If all the profiles in the IOR fail to connect then a 
> TRANSIENT exception is thrown from the selector. Because this 
> occurs before the send_request interception point has (potentially) 
> been called this exception is always returned directly to the 
> client and does not trigger the receive_exception interception 
> point.

Right, and this is compliant with the spec. 

> This doesn't seem right. 

Why?

> It is certainly inconsistent from a user perspective in that 
> if a server that stops running before the try_connect that precedes the
> transmission of the first message receive_exception is not called but
> a failure after the try_connect can be handled by receive_exception. 

I don't get what you mean here. 

> Both of these scenarios appear ORB mediated to the user 
> in that control has disappeared from the application and into 
> the stub request sending code.

Yes, it has. Until send_request () is called nothing else needs to be done. TAO
calls later and it is compliant. I don't understand the user perspective in your
report. 

> The rather jolly CORBA Metaprogramming Mechanisms, Part 1: 
> Portable Interceptors Concepts and Components suggests that 
> interceptors can be used to provide FT and load balancing but 
> this behaviour makes this sort of thing almost impossible as it 
> is not feasible for an interceptor to re-route a request with a 
> location forward in the event that an existing IOR is now unreachable.

I am not sure what is written in that book, but looks like there may be a biased
view. If the primary in FT case is not available the spec suggests to contact
secondary which could locate forward to the new primary of any. The LB works in
a similar way too. The ORB already has a list of profile to try. If they are
unreachable applications need to be told about it even in FT case. Without the
interception point invoked clients know that the references (since the request
never even hit the wire) are useless and needs to contact reference store to
grab a new if any. 

I don't think TAO developers should be accepting this patch. 

BTW, FT and LB are about making services (aka servants/servers) available, not
to make the client keep trying until they burn their CPU's. Looks like invoking
interceptors to throw LocateForward loops would be just that

Hi Bala. Thanks for replying. I felt sure this behaviour wasn't an oversight so
I was hoping someone would step in and explain the alternate viewpoint so we
could have a discussion about it.

>>This doesn't seem right. 
> 
> 
> Why?
> 
> 
>>It is certainly inconsistent from a user perspective in that 
>>if a server that stops running before the try_connect that precedes the
>>transmission of the first message receive_exception is not called but
>>a failure after the try_connect can be handled by receive_exception. 
> 
> 
> I don't get what you mean here. 

Then I should try to explain. Design principle #1 of request interceptors states
that "Interceptors are called on all ORB mediated invocations." I would argue
that if a user has successfully narrowed an object reference and then goes on to
call: 

my_ref->my_operation (ACE_ENV_SINGLE_ARG_PARAMETER);

... on it, then that will constitute an orb mediated invocation and interceptors
should be called.

As I think you point out there is nothing in the spec to say that the ORB cannot
try and connect before calling the inteception points but equally there is
nothing in the spec that says that an orb should do this. TAO has made a choice
which isn't wrong as per the spec but which I believe is restrictive unecessarily.

If the profile(s) in the object reference is unreachable for whatever reason
before the try_connect takes place then no interception points are called. If
the server becomes unreachable after the try_connect and either before or after
the actual transmission of the first message then both send_request and
receive_exception are called. The try_connect happens at some arbitrary point in
time between control disappearing from the user and the first message making it
onto the wire.

>>The rather jolly CORBA Metaprogramming Mechanisms, Part 1: 
>>Portable Interceptors Concepts and Components suggests that 
>>interceptors can be used to provide FT and load balancing but 
>>this behaviour makes this sort of thing almost impossible as it 
>>is not feasible for an interceptor to re-route a request with a 
>>location forward in the event that an existing IOR is now unreachable.
> 
> 
> I am not sure what is written in that book, but looks like there may be a biased
> view.

The authors are Douglas C. Schmidt and Steve Vinoski. I can't vouch for whether
they're fair minded or not. :-)

> If the primary in FT case is not available the spec suggests to contact
> secondary which could locate forward to the new primary of any. The LB works in
> a similar way too. The ORB already has a list of profile to try. If they are
> unreachable applications need to be told about it even in FT case. Without the
> interception point invoked clients know that the references (since the request
> never even hit the wire) are useless and needs to contact reference store to
> grab a new if any. 

I maybe shouldn't have mentioned FT - it's clouding the issue. I'm familiar with
how OMG defined FT is supposed to work.

There are many ways to skin a cat and some users may want to tackle the problem
differently. Let's suppose a user writes an interceptor that is able to obtain
alternate locations from somewhere and wishes to forward requests to one of the
alternates whenever there is a failure. It's not full blown FT but maybe it's
good enough for them. Or maybe they want to write an interceptor that logs all
communication problems when an object is unreachable. Neither of these are
possible at present.

> BTW, FT and LB are about making services (aka servants/servers) available, not
> to make the client keep trying until they burn their CPU's. Looks like invoking
> interceptors to throw LocateForward loops would be just that
> 

Let's give users some credit. If they choose to write interceptors that cause
LocateForward loops then surely that's their lookout. Maybe they want to burn
out their CPUs. It's their CPU - they should be able to do what they want with
it. :-)

> I don't think TAO developers should be accepting this patch.
> 

My point of view remains that I can't see what is wrong with letting users'
interceptors have the chance to look at all exceptions on an invocation rather
than just a subset. It doesn't really matter though - this functionality has
gone into our versions and it will be staying there. If public opinion is
against it I'm happy for us to not merge this back and maintain it seperately.

Hi Simon,

Some background information ...

Back in the early days of TAO's PI support, the connection attempt was made
after send_request() interception point was invoked.  Soon after I fixed the
implementation to conform to the PI "General Flow Rules" defined by the spec,
the connection attempt was moved to just before the send_request() interception
point.  

This was done as an optimization, not to make TAO more spec compliant.  The goal
was to prevent a socket connection, for example, from being established before
send_request() since send_request() may throw an exception (system exception or
ForwardRequest), rendering the existing connection potentially useless.

That said, I'm okay with reverting to the earlier semantics, which is apparently
what your patches do, if everyone is okay with losing the optimization.

*sigh*  I'm not making any sense, am I?  :-)

Corrections:

- send_request() was originally done after the connection was established.
- later, send_request() was moved before the connection attempt.
- it now appears that send_request() once again occurs after the connection has
been established, so we lost the optimization.

Bala, do you know why the connection establishment was moved back before the
send_request() interception point?

I'm now in favor of Simon's patches since it looks like they put things back the
way I intended.  :-)

Let me do LIFO. 

> Bala, do you know why the connection establishment was moved 
> back before the send_request() interception point?

Firstly, the optimization that is talked about is probably good for a subest of
usecases. Users whom I work with generally do a bunch of things in the
interception point, like adding special contexts for authentication and run
other queries. Calling application and wasting CPU when the ORB is not even sure
whether it is going to send out a request or not is very useful IMHO. 

Secondly, without holding up a connection, request_id () in send_request () will
be perenially wrong, especially for TAO. 

Thirdly, this prevents possible extensions that I had thought for a long time.
For example, TAO could implement a TransportCurrent, which one could potentially
grab from the ORB (using resolve_**). The interception point could query the
port and the operation name and send the information to a management console to
display operations, and size of data flowing on different endpoints. Pushing it
back to pre-connection, I will be totally hosed. I wouldn't know the endpoint
nor the request ID.  

I have atleast 4 more usecases (all management type and monitoring type of
applications) which I think are useful for "large scale" enterprise wide
applications. Enabling them would be (and were) more important for me than
receiving exceptions on connection failure and subsequent optimizations. 

Lets look at next:

> Design principle #1 of request interceptors states 
> that "Interceptors are called on all ORB mediated invocations." 
> I would argue that if a user has successfully narrowed an 
> object reference and then goes on to call: 

> my_ref->my_operation (ACE_ENV_SINGLE_ARG_PARAMETER);

> ... on it, then that will constitute an orb mediated invocation 
> and interceptors should be called.

Feel free to argue :-), only that some of the information (that users could get
in the interception point) is wrong, and potentially more useful info can never
be got. You make it sound as if that we should call interceptors as soon the
code enters the stubs for world peace :-). I am trying to point out there are
more elements to world piece than just this. 

> As I think you point out there is nothing in the spec to say 
> that the ORB cannot try and connect before calling the 
> inteception points but equally there is nothing in the spec 
> that says that an orb should do this. TAO has made a choice which 
> isn't wrong as per the spec but which I believe is restrictive 
> unecessarily.

Please see above. I have clearly pointed out usecases, application categories,
where doing this is more useful and satisfies a wide range of requirements.
Infact, my sense is that the present scheme in TAO is more flexible and with
some nice extensions are more useful. 

> The authors are Douglas C. Schmidt and Steve Vinoski. I can't 
> vouch for whether they're fair minded or not. :-)

No comments ;). But I know for sure that they were not too concerned about TAO's
implementation details, when they wrote the article, like we do. 

> There are many ways to skin a cat and some users may want to 
> tackle the problem differently. Let's suppose a user writes 
> an interceptor that is able to obtain alternate locations from 
> somewhere and wishes to forward requests to one of the alternates 
> whenever there is a failure. 

You make it sound as if interceptors keep jumping out of MARS. Application
developers code them. If the interceptors can know what the alternate locations
are, so would the rest of the application. If they want to put common code
somewhere, they could write extra classes that does the job more cleanly. Looks
like a weak argument to me. 

> It's not full blown FT but maybe it's good enough for them. Or maybe 
> they want to write an interceptor that logs all communication problems 
> when an object is unreachable. Neither of these are possible at present.

There is more to communication problems than just calling connect () at the 
TCP/IP level (or SSL or whatever). If there are 5 threads atmost connects will
be called a handful of times? So you want to help your application writers with
this, compromising other useful usecases. Looks like weak argument 

> It doesn't really matter though - this functionality has 
> gone into our versions and it will be staying there. If public 
> opinion is against it I'm happy for us to not merge this back 
> and maintain it seperately.

I can surely see your point. 

I have tried to provide a strong reason why the approach that Simon suggests is
not scalable. I leave it to the rest of TAO developers to decide. I am tempted
to mark it as WONTFIX, but I leave it to others to decide.  

1. CPU resources is a non-issue.  We're talking about operations that are
relatively quick.  If interception point operations were computation intensive,
I might agree, but they are not.

2. The request_id() issue you refer to is a non-issue.  The GIOP request ID and
the request ID returned from PortableInterceptor::RequestInfo::request_id() are
not the same.  An ORB implementation may choose to make them same but that is
not so in TAO's case.  The spec only requires that the request ID returned from
PortableInterceptor::RequestInfo::request_id() be unique across concurrent
invocations, which is what TAO guarantees without relying on the GIOP request ID.

3. You could still get TransportCurrent working and the optimization I want by
placing the send_request() interception point between the endpoint selection and
connection establishment.  The request ID issue is again a non-issue if you use
RequestInfo::request_id().  If for some reason you don't want to do that, you
could still get the request ID before send_request().  Of course, we'll have to
tweak the ORB a bit.

4. Something that may be useful for you may not be useful for others, so let's
stay away from the "weak argument" rhetoric.  It's not constructive.  Both you
and Simon have valid points, and I don't yet see why we all can't get what we want.

I agree with everything in Ossama's post. Particularly the last statement.

Shall we take a step back and look at where we are ?

If, as Ossama suggests, we disregard the "wasted CPU cycles when a request is
going to crap out anyway" issue I think we are left with: 

Bala would like the the transport to be connected before the send_request
interception point so he has the option of making use of it in an extension he
may write.

Ossama would like the send_request interception point to be called before the
connect attempt to optimise the case where the interceptor wishes to route the
request elsewhere.

Simon would like to be able to receive failed connection exceptions in the
receive_exception interception point to make his customers' portable interceptor
based applications work.

With the current implementation: Bala is happy, Ossama and Simon are not.

Ossama's proposal is move the send_request before the connect. Ossama and Simon
would be happy, Bala would not be.

What my original patch did was to leave the connect where it is and delay
generating the exception from the failed connect until after the send_request
interception point. Obviously Simmon would be happy or he wouldn't have
suggested it. If Bala would be willing to test whether the transport is actually
there before trying to use it when he comes to write his extension then I think
this would satisfy his requirement. It does not address Ossama's requirement at
all however. 

If we can't agree on what is the right thing maybe we could consider making the
connection point switchable based on a configuration option ? I would be quite
happy for my take on things to be the non-default behaviour as long it is still
possible somehow.

Thanks for summarizing where we stand!

Bala, do you need the connection to be established in order to implement
TransportCurrent and other related extension?  Assuming that the
PortableInterceptor request ID is suitable for you, it seems possible to
implement them without the connection as long as you have endpoint information.
 Is that correct?  If so, I can think all of our interests can be satisfied. 
Placing send_request() between endpoint selection and connection establishment
also gives another opportunity for monitoring/logging that you do not have with
the existing code.

Simon, would this work for you, too?

That would be fine by me.

Hi Folks, I would really like to get convergence on this.  It sounds like the 
latest suggestion by Ossama will address Simon's concerns, and may also 
address the bulk of Bala's concerns.  Bala, do you have any problems with 
Ossama's suggestion?  If not, let's go ahead and do this.  If so, let's see 
whether we can make the behavior "pluggable".  Thanks!

Xref: Scarab TAO#214.

Accepting

changed component to PI

added blocks

Simon, your patch is very small, but I don't see a real reason anymore why in Invocation_Adapter::invoke_remote_i we then need to use the Profile_Transport_Resolver, why not move the object on the stack and Profile_Transport_Resolver::resolve() further in the call chain, that way you don't have to change the Profile_Transport_Resolver, we don't have a delay when no transport is there before calling send_request. Ok, it will break some internal interfaces but that shouldn't be much work

Hi Johnny - it's over two years since I looked at this code so I have no objection to anything that you think would work well instead. If there ever was a convincing reason for my patch being the way it was it may well no longer be valid.

taking this over. This needs a little bit of rework, the original patch will cause a crash. I will move the usage of the IES to a later moment in the call chain

patch doesn't work on svn head, also it seems messaging needs also an update

Simon, do you have a regression test for this one from your side? I have patches ready which fix bug 3079 and bug 3080 but it would be nice to have also a test from your side

Simon, do you have tested your patch with AMI?

DII also has problems when there is no transport, patch will break that support also. The Transport Current tests do also fail.

Created attachment 866 [details]
Proposed patch to move connection establishment after send_request point

updated version

changelog for the patch added


          Changed the way how portable interceptors relate to connection
          establishment. A few years ago the connection was setup after the
          send_request client interceptor call, this was changed with some
          refactoring. The send_request was now called after the connection
          establishment, as a result, if the connection establishment failed,
          the receive_exception method on the client request interceptor
          wasn't called. With this behaviour it is not possible to implement
          a FT app with just interceptors, you need a TAO specific endpoint
          selector. The change I made is to move the connection establishment
          after the send_request. As a result in the send_request it can
          happen that there is no transport selected, at that moment
          retrieving transport current (which is TAO specific) will give a
          NoContext exception.

          This fixes bugzilla 2133, 3079, and 3080. Thanks to Simon McQueen
          and Jaiganesh Balasubramanian for reporting these issues.

        * tao/Synch_Invocation.cpp:
        * tao/Synch_Invocation.h:
        * tao/Messaging/Asynch_Invocation.cpp:
          Moved the resolve on the profile_transport_resolver to after the
          send_request. At the moment a transport has been selected the
          virtual method transport_resolved() is called so that derived
          classes can set additional values on the transport.

        * tao/Remote_Invocation.{h,cpp} (write_header):
          Setup TAO_Target_Specification within this method, no need to
          pass it in as argument

        * tao/DynamicInterface/DII_Invocation.{h,cpp}:
          Use the new transport_resolved to set the transport in the safe_rd and
          reset the byter order, at the moment the remote_invocation is called
          it is possible that the transport is not resolved yet

        * tao/DynamicInterface/DII_Invocation_Adapter.cpp:
          Don't reset the byte order here, do that in DII_Invocation

        * tao/Invocation_Adapter.cpp:
          Const changes, layout changes and don't resolve the transport here, do
          that in the Remote_Invocation derived classes

        * tao/Messaging/Asynch_Invocation_Adapter.cpp:
          Don't schedule the timer here, but in the Async_Invocation because we
          maybe haven't resolved the transport.

        * tao/TransportCurrent/IIOP_Current_Impl.cpp:
          When we have no transport or no connection handler return a NoContext
          instead of No_Implement

        * tao/Object.cpp:
          Fixed typo in comment

        * tests/TransportCurrent/Framework/Current_Test_Impl.cpp:
        * tests/TransportCurrent/lib/Client_Request_Interceptor.cpp:
        * tests/TransportCurrent/lib/Client_Request_Interceptor.h:
          Updated for the fact that send_request could throw NoContext

Sorry Johnny - have only just caught up with this ticket now. As you've probably seen I've already just mailed you the regression test in another conversation.

Wed Nov  7 18:48:15 UTC 2007  Johnny Willemsen  <jwillemsen@remedy.nl>

        * tests/Portable_Interceptors/Bug_2133/*:
          New regression test for bug 2133, thanks to Simon McQueen for
          creating this test

Mon Nov 12 08:54:21 UTC 2007  Johnny Willemsen  <jwillemsen@remedy.nl>

reopening, will rework my patch to get better support for TC during send_request

Tue Nov 27 11:20:54 UTC 2007  Johnny Willemsen  <jwillemsen@remedy.nl>

        * tao/DynamicInterface/DII_Invocation.cpp:
        * tao/DynamicInterface/DII_Invocation.h:
        * tao/DynamicInterface/DII_Invocation_Adapter.cpp:
        * tao/Invocation_Adapter.cpp:
        * tao/Invocation_Endpoint_Selectors.cpp:
        * tao/Messaging/Asynch_Invocation.cpp:
        * tao/Messaging/Asynch_Invocation_Adapter.cpp:
        * tao/Profile_Transport_Resolver.cpp:
        * tao/Synch_Invocation.cpp:
        * tao/Synch_Invocation.h:
          Reworked the fix for bug 2133/3079/3080 in such a way that the
          Transport Current code is working again (see bug 3127)