diff options
author | bala <balanatarajan@users.noreply.github.com> | 2002-12-22 15:16:47 +0000 |
---|---|---|
committer | bala <balanatarajan@users.noreply.github.com> | 2002-12-22 15:16:47 +0000 |
commit | a356b1a69f2226212f728b8445f5f974f5d88e91 (patch) | |
tree | c96ba34bb175b2b7d1c857fe1515cb12b6241b9a | |
parent | 7eb011fb510be675b27deb68e2c55c022a301e99 (diff) | |
download | ATCD-a356b1a69f2226212f728b8445f5f974f5d88e91.tar.gz |
ChangeLogTag:Sun Dec 22 09:18:00 2002 Balachandran Natarajan <bala@isis-server.isis.vanderbilt.edu>
-rw-r--r-- | ChangeLog | 23 | ||||
-rw-r--r-- | ChangeLogs/ChangeLog-03a | 23 | ||||
-rw-r--r-- | ace/Connector.cpp | 206 | ||||
-rw-r--r-- | ace/Connector.h | 30 |
4 files changed, 229 insertions, 53 deletions
diff --git a/ChangeLog b/ChangeLog index 8dc658791b6..ef9cac7a1da 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,26 @@ +Sun Dec 22 09:18:00 2002 Balachandran Natarajan <bala@isis-server.isis.vanderbilt.edu> + + * ace/Connector.cpp: + * ace/Connector.h: Added a lock to the ACE_Connector class to + make the following atomic + + . Registration of AST with the handler_map_ + . Registration of the ACE_Connector with the Reactor + . and the registration of timers with the Reactor's timer + queue. + + If they are not atomic, it throws open race conditions such as + the ones documented in BUG 1405. This is not complete fix for + BUG 1405, since it taxes applications using only one thread with a + lock. This is a short-term fix to prevent the race condition + from occuring. + + Other relevant changes include + + . the AST is now refcounted + . the AST cannot be deleted directly. The lifetime of the AST + can only be manipulated using the refcount on the AST. + Sat Dec 21 18:27:42 2002 Steve Huston <shuston@riverace.com> * ace/ace_dll.vcp: diff --git a/ChangeLogs/ChangeLog-03a b/ChangeLogs/ChangeLog-03a index 8dc658791b6..ef9cac7a1da 100644 --- a/ChangeLogs/ChangeLog-03a +++ b/ChangeLogs/ChangeLog-03a @@ -1,3 +1,26 @@ +Sun Dec 22 09:18:00 2002 Balachandran Natarajan <bala@isis-server.isis.vanderbilt.edu> + + * ace/Connector.cpp: + * ace/Connector.h: Added a lock to the ACE_Connector class to + make the following atomic + + . Registration of AST with the handler_map_ + . Registration of the ACE_Connector with the Reactor + . and the registration of timers with the Reactor's timer + queue. + + If they are not atomic, it throws open race conditions such as + the ones documented in BUG 1405. This is not complete fix for + BUG 1405, since it taxes applications using only one thread with a + lock. This is a short-term fix to prevent the race condition + from occuring. + + Other relevant changes include + + . the AST is now refcounted + . the AST cannot be deleted directly. The lifetime of the AST + can only be manipulated using the refcount on the AST. + Sat Dec 21 18:27:42 2002 Steve Huston <shuston@riverace.com> * ace/ace_dll.vcp: diff --git a/ace/Connector.cpp b/ace/Connector.cpp index abc0e976235..e55848ff930 100644 --- a/ace/Connector.cpp +++ b/ace/Connector.cpp @@ -156,11 +156,17 @@ ACE_Svc_Tuple<SVC_HANDLER>::ACE_Svc_Tuple ( : svc_handler_ (sh), handle_ (handle), arg_ (arg), - cancellation_id_ (id) + cancellation_id_ (id), + refcount_ (1) { ACE_TRACE ("ACE_Svc_Tuple<SVC_HANDLER>::ACE_Svc_Tuple"); } +template <class SVC_HANDLER> +ACE_Svc_Tuple<SVC_HANDLER>::~ACE_Svc_Tuple (void) +{ +} + template <class SVC_HANDLER> SVC_HANDLER * ACE_Svc_Tuple<SVC_HANDLER>::svc_handler (void) { @@ -210,6 +216,27 @@ ACE_Svc_Tuple<SVC_HANDLER>::cancellation_id (long id) this->cancellation_id_ = id; } +template <class SVC_HANDLER> long +ACE_Svc_Tuple<SVC_HANDLER>:: incr_refcount (void) +{ + ACE_TRACE ("ACE_Svc_Tuple<SVC_HANDLER>::incr_refcount"); + return ++this->refcount_; +} + +template <class SVC_HANDLER> long +ACE_Svc_Tuple<SVC_HANDLER>:: decr_refcount (void) +{ + ACE_TRACE ("ACE_Svc_Tuple<SVC_HANDLER>::decr_refcount"); + if (--this->refcount_ > 0) + return this->refcount_; + + ACE_ASSERT (this->refcount_ == 0); + + delete this; + + return 0; +} + template <class SVC_HANDLER> void ACE_Svc_Tuple<SVC_HANDLER>::dump (void) const { @@ -233,10 +260,15 @@ ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::handle_timeout ( { ACE_TRACE ("ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::handle_timeout"); AST *ast = 0; + int retval = 0; if (this->cleanup_AST (((AST *) arg)->handle (), ast) == -1) - return -1; + { + // Matches the creation time refcount for AST, which is 1 + this->decr_ast_refcount ((AST *) arg); + return -1; + } else { ACE_ASSERT (((AST *) arg) == ast); @@ -252,7 +284,8 @@ ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::handle_timeout ( if (sh->handle_timeout (tv, ast->arg ()) == -1) sh->handle_close (sh->get_handle (), ACE_Event_Handler::TIMER_MASK); - delete ast; + // Matches the creation time refcount for AST, which is 1 + this->decr_ast_refcount (ast); return 0; } } @@ -264,31 +297,44 @@ ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::cleanup_AST ( { ACE_TRACE ("ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::cleanup_AST"); + ACE_MT (ACE_GUARD_RETURN (ACE_SYNCH_MUTEX, + ace_mon, + this->mutex_, + -1)); + // Locate the ACE_Svc_Handler corresponding to the socket // descriptor. - if (this->handler_map_.find (handle, ast) == -1) + if (this->handler_map_.unbind (handle, ast) == -1) { // Error, entry not found in map. errno = ENOENT; - ACE_ERROR_RETURN ((LM_ERROR, - ACE_LIB_TEXT ("ACE (%P|%t) - Connector<>::cleanup_AST") - ACE_LIB_TEXT ("%p %d not found in map\n"), - ACE_LIB_TEXT ("find"), - handle), - -1); + return -1; } + // Matches incr_refcount () in create_AST () after registering + // with the map. + ast->decr_refcount (); + // Try to remove from ACE_Timer_Queue but if it's not there we // ignore the error. - this->reactor ()->cancel_timer (ast->cancellation_id ()); + if (this->reactor ()->cancel_timer (ast->cancellation_id ())) + { + // Matches incr_refcount () in create_AST () after registering + // the timer + ast->decr_refcount (); + } - // Remove ACE_HANDLE from ACE_Reactor. - this->reactor ()->remove_handler - (handle, ACE_Event_Handler::ALL_EVENTS_MASK | ACE_Event_Handler::DONT_CALL); - // Remove ACE_HANDLE from the map. - this->handler_map_.unbind (handle); + ACE_Reactor_Mask m = + ACE_Event_Handler::ALL_EVENTS_MASK | ACE_Event_Handler::DONT_CALL; + // Remove ACE_HANDLE from ACE_Reactor. + if (this->reactor ()->remove_handler (handle, m) == 0) + { + // Matches incr_refcount () in create_AST () after registering + // with the Reactor. + ast->decr_refcount (); + } return 0; } @@ -305,8 +351,11 @@ ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::handle_input (ACE_HANDLE h) { ACE_ASSERT (ast != 0); ast->svc_handler ()->close (0); - delete ast; + + // Matches the creation time refcount for AST, which is 1 + this->decr_ast_refcount (ast); } + return 0; // Already removed from the ACE_Reactor. } @@ -323,8 +372,9 @@ ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::handle_output (ACE_HANDLE hand AST *ast = 0; if (this->cleanup_AST (handle, ast) == -1) - return 0; - + { + return 0; + } ACE_ASSERT (ast != 0); // This shouldn't happen! // Try to find out if the reactor uses event associations for the @@ -357,7 +407,8 @@ ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::handle_output (ACE_HANDLE hand #endif /* ACE_WIN32 */ ast->svc_handler ()->close (0); } - delete ast; + // Matches the creation time refcount for AST, which is 1 + this->decr_ast_refcount (ast); return 0; } @@ -556,8 +607,10 @@ ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::cancel (SVC_HANDLER *sh) if (me->int_id_->svc_handler () == sh) { AST *ast = 0; - (void) this->cleanup_AST (me->ext_id_, ast); - delete ast; + + if (this->cleanup_AST (me->ext_id_, ast) != -1) + this->decr_ast_refcount (ast); + return 0; } @@ -573,9 +626,16 @@ ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::create_AST ( const ACE_Synch_Options &synch_options) { ACE_TRACE ("ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::create_AST"); + + ACE_MT (ACE_GUARD_RETURN (ACE_SYNCH_MUTEX, + ace_mon, + this->mutex_, + -1)); + ACE_HANDLE handle = sh->get_handle (); AST *ast; + // AST is created with a refcount ACE_NEW_RETURN (ast, AST (sh, handle, @@ -588,33 +648,48 @@ ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::create_AST ( // Bind ACE_Svc_Tuple with the ACE_HANDLE we're trying to connect. if (this->handler_map_.bind (handle, ast) == -1) goto fail1; - else if (this->reactor ()->register_handler (handle, - this, - mask) == -1) + + // Increment the refcount of the AST to indicate registration with + // the map. + ast->incr_refcount (); + + if (this->reactor ()->register_handler (handle, + this, + mask) == -1) goto fail2; - // If we're starting connection under timer control then we need to - // schedule a timeout with the ACE_Reactor. - else - { - ACE_Time_Value *tv = - ACE_const_cast (ACE_Time_Value *, - synch_options.time_value ()); - if (tv != 0) - { - int cancellation_id = - this->reactor ()->schedule_timer - (this, - (const void *) ast, - *tv); - if (cancellation_id == -1) - goto fail3; - - ast->cancellation_id (cancellation_id); - return 0; - } - else - return 0; // Ok, everything worked just fine... - } + + // Increment the refcount of the AST. This increment is for access + // from the Reactor. Though we register <this> with the Reactor, + // every dispatch from the Reactor actually looks for <ast> and + // hence the refcount increment. + (void) ast->incr_refcount (); + + { + // If we're starting connection under timer control then we need to + // schedule a timeout with the ACE_Reactor. + ACE_Time_Value *tv = + ACE_const_cast (ACE_Time_Value *, + synch_options.time_value ()); + if (tv != 0) + { + int cancellation_id = + this->reactor ()->schedule_timer + (this, + (const void *) ast, + *tv); + if (cancellation_id == -1) + goto fail3; + + // Increment the refcount of the AST. This increment is for access + // from the timer queue. The same argument used for Rector + // registration holds true here too. + (void) ast->incr_refcount (); + + ast->cancellation_id (cancellation_id); + return 0; + } + } + return 0; // Ok, everything worked just fine... // Undo previous actions using the ol' "goto label and fallthru" // trick... @@ -630,7 +705,7 @@ fail1: // Close the svc_handler sh->close (0); - delete ast; + ast->decr_refcount (); return -1; } @@ -680,10 +755,12 @@ ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::handle_close (ACE_HANDLE, ACE_ { ACE_ASSERT (ast != 0); ast->svc_handler ()->close (0); + + // Zap the ast. + this->decr_ast_refcount (ast); } - // Zap the ast. - delete ast; + } } @@ -753,6 +830,33 @@ ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::~ACE_Connector (void) this->handle_close (); } +template <class SVC_HANDLER, ACE_PEER_CONNECTOR_1> long +ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::incr_ast_refcount ( + ACE_Svc_Tuple<SVC_HANDLER> *ast) +{ + ACE_TRACE ("ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::incr_ast_refcount"); + ACE_MT (ACE_GUARD_RETURN (ACE_SYNCH_MUTEX, + ace_mon, + this->mutex_, + -1)); + + return ast->incr_refcount (); +} + +template <class SVC_HANDLER, ACE_PEER_CONNECTOR_1> long +ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::decr_ast_refcount ( + ACE_Svc_Tuple<SVC_HANDLER> *ast) +{ + ACE_TRACE ("ACE_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::decr_ast_refcount"); + ACE_MT (ACE_GUARD_RETURN (ACE_SYNCH_MUTEX, + ace_mon, + this->mutex_, + -1)); + + return ast->decr_refcount (); +} + +/***********************************************************/ template <class SVC_HANDLER, ACE_PEER_CONNECTOR_1> int ACE_Strategy_Connector<SVC_HANDLER, ACE_PEER_CONNECTOR_2>::open (ACE_Reactor *r, int flags) { diff --git a/ace/Connector.h b/ace/Connector.h index d3b74615043..2c938c8a2a5 100644 --- a/ace/Connector.h +++ b/ace/Connector.h @@ -69,12 +69,22 @@ public: /// Set cancellation id. void cancellation_id (long timer_id); + /// Increment and decrement refcount within the context of the lock + /// on the ACE_Connector + long incr_refcount (void); + + long decr_refcount (void); + /// Dump the state of an object. void dump (void) const; /// Declare the dynamic allocation hooks. ACE_ALLOC_HOOK_DECLARE; +protected: + /// Prevent direct deletion + ~ACE_Svc_Tuple (void); + private: /// Associated SVC_HANDLER. SVC_HANDLER *svc_handler_; @@ -87,6 +97,11 @@ private: /// Associated cancellation id. long cancellation_id_; + + /// Reference count manipulated within the context of the connector + /// lock. + /// @@ TODO: Things will change after 5.3 goes out of the way. + long refcount_; }; /** @@ -352,9 +367,15 @@ protected: int flags, int perms); + + /// Helper method for manipulating the refcount on AST. It holds the + /// lock before manipulating the refcount on AST. + /// @@ TODO: Needs to be out after 5.3 + long incr_ast_refcount (AST *ast); + long decr_ast_refcount (AST *ast); + /// Lookup table that maps an I/O handle to a SVC_HANDLER *. MAP_MANAGER handler_map_; - private: /// This is the concrete connector factory (it keeps no state so the /// <ACE_Connector> is reentrant). @@ -371,6 +392,11 @@ private: * the <SVC_HANDLER> when it is opened. */ int flags_; + + /// Lock to synchronize access to the internal state of the + /// connector. + /// @@TODO: This needs to go after 1.3 + ACE_SYNCH_MUTEX mutex_; }; /** @@ -396,7 +422,7 @@ public: // Useful STL-style traits. typedef ACE_Creation_Strategy<SVC_HANDLER> creation_strategy_type; - typedef ACE_Connect_Strategy<SVC_HANDLER, ACE_PEER_CONNECTOR_2> + typedef ACE_Connect_Strategy<SVC_HANDLER, ACE_PEER_CONNECTOR_2> connect_strategy_type; typedef ACE_Concurrency_Strategy<SVC_HANDLER> concurrency_strategy_type; |