Bug 2511 - If ServantLocator::preinvoke() throws Exceptions, postinvoke is called
Summary: If ServantLocator::preinvoke() throws Exceptions, postinvoke is called
Status: RESOLVED FIXED
Alias: None
Product: TAO
Classification: Unclassified
Component: POA (show other bugs)
Version: 1.5
Hardware: x86 Windows XP
: P3 normal
Assignee: DOC Center Support List (internal)
URL:
Depends on: 1592
Blocks: 2658
  Show dependency tree
 
Reported: 2006-04-21 05:28 CDT by Martin Cornelius
Modified: 2007-02-21 06:35 CST (History)
0 users

See Also:


Attachments
patch created by diff that fixes the bug (747 bytes, patch)
2006-04-21 05:29 CDT, Martin Cornelius
Details
Test case the exposes the bug (4.57 KB, application/zip)
2006-04-26 07:49 CDT, Martin Cornelius
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Cornelius 2006-04-21 05:28:46 CDT
If ServantLocator::preinvoke() throws Exceptions, postinvoke() must not be 
called according to the COARBA-2.6 spec, $11.3.6.

With TAO-1.5, postinvoke is called in this case, with parameter operation set 
to 0. Even worse IMHO (although the spec is not very clear about this), if 
preinvoke() returns 0, meaning no servant was found,  postinvoke() is called as 
well.

My investigation revealed the following:

TAO_Object_Adapter::dispatch_servant() creates a Servant_Upcall instance and 
calls Servant_Upcall::prepare_for_upcall()
This method finally calls 
RequestProcessingStrategyServantLocator::locate_servant()
, where the follwing happens in this order:
 - Servant_Upcall::state (OBJECT_ADAPTER_LOCK_RELEASED);
 - ServantLocator::preinvoke()
 - Servant_Upcall::operation(operation)

if ServantLocator::preinvoke() throws an Execption, the destructor of 
Servant_Upcall is called when the stack is unwound, and this destructor calls
Servant_Upcall::upcall_cleanup().

This method contains a switch on the member state_.

As state_ has been set to OBJECT_ADAPTER_LOCK_RELEASED (see above),
ServantLocator::postinvoke() is now called with NULL operation.

The case when ServantLocator::preinvoke() returns NULL is very similar...

My quick hack to fix this problem: 
In Servant_Upcall::upcall_cleanup(), call ServantLocator::postinvoke() only if 
operation_ and servant_ are both not NULL.

--------------------- snip snip -----------------------

diff -r -u PortableServer.orig/Servant_Upcall.cpp 
PortableServer/Servant_Upcall.cpp
--- PortableServer.orig/Servant_Upcall.cpp	2006-02-13 04:23:52.000000000 
+0100
+++ PortableServer/Servant_Upcall.cpp	2006-04-21 11:00:18.480507500 +0200
@@ -288,7 +288,10 @@
           // Cleanup servant locator related state.  Note that because
           // this operation does not change any Object Adapter related
           // state, it is ok to call it outside the lock.
-          this->post_invoke_servant_cleanup ();
+          if ( this->operation() && this->servant() )
+          {
+            this->post_invoke_servant_cleanup ();
+          }
 
           // Since the object adapter lock was released, we must acquire
           // it.

--------------------- snip snip -----------------------
Comment 1 Martin Cornelius 2006-04-21 05:29:17 CDT
Created attachment 529 [details]
patch created by diff that fixes the bug
Comment 2 Johnny Willemsen 2006-04-23 06:37:46 CDT
to normal pool
Comment 3 Johnny Willemsen 2006-04-23 06:38:11 CDT
can you provide also a test program for this one, makes it easier to analyze and
makes sure things don't get introduced back in the future
Comment 4 Martin Cornelius 2006-04-26 07:49:06 CDT
Created attachment 534 [details]
Test case the exposes the bug
Comment 5 Johnny Willemsen 2006-04-26 07:59:48 CDT
I will add the regression to cvs soon
Comment 6 Johnny Willemsen 2006-04-26 08:33:16 CDT
accept, test is under tests/POA
Comment 7 Johnny Willemsen 2006-06-08 06:59:51 CDT
added dependency on another postinvoke bug
Comment 8 Johnny Willemsen 2007-02-21 05:56:05 CST
added blocks
Comment 9 Johnny Willemsen 2007-02-21 06:35:06 CST
Wed Feb 21 12:23:12 UTC 2007  Johnny Willemsen  <jwillemsen@remedy.nl>

        * tests/POA/Bug_2511_Regression/server.cpp:
          Return the error count, in case of a failure then the test
          framework will notice this

        * tao/PortableServer/RequestProcessingStrategyServantLocator.cpp:
          Don't call postinvoke when we don't have a servant. Fixes bugzilla
          bugs 2511 and 2658. Thanks to Martin Cornelius
          <Martin dot Cornelius at smiths-heimann dot com> and Milan
          Cvetkovic <milan dot cvetkovic at mpathix dot com> for reporting
          this and providing a test case and a proposed fix.
Comment 10 Johnny Willemsen 2007-02-21 06:35:14 CST
fixed