ChangeLogTag: Fri Mar 21 21:13:32 UTC 2008 Iliyan Jeliazkov <iliyan@ociweb.com>

2 files changed, 72 insertions, 49 deletions

diff --git a/ACE/ChangeLog.iliyan-gestalt b/ACE/ChangeLog.iliyan-gestalt
index 5341e30c862..9e1c458f655 100644
--- a/ACE/ChangeLog.iliyan-gestalt
+++ b/ACE/ChangeLog.iliyan-gestalt
@@ -1,3 +1,20 @@
+Fri Mar 21 21:13:32 UTC 2008  Iliyan Jeliazkov  <iliyan@ociweb.com>
+
+        Trying to be frugal and save space by packing the SR after a call
+        to remove() causes problems with relocating dependent
+        services. For the relocation to work, we cache the index where the
+        next service will be inserted. If something, in the mean time
+        removes a service (and packs the storage) it might prevent some
+        services from relocation. In turn, it causes finalization problems
+        because the non-relocated services may end up in prematurely
+        unmapped memory. See TAO/tests/DLL_ORB for use case.
+
+        * ace/Service_Repository.cpp:
+
+        Changed remove() to eliminate the "packing" code and updated the
+        few other methods, which assumed there are no "gaps" in the
+        service storage.
+
 Thu Mar 20 19:42:06 UTC 2008  Iliyan Jeliazkov  <iliyan@ociweb.com>
 
         The legacy implementation of the SR ordered the services, based on
diff --git a/ACE/ace/Service_Repository.cpp b/ACE/ace/Service_Repository.cpp
index 88a24dda64b..f6aabeb0195 100644
--- a/ACE/ace/Service_Repository.cpp
+++ b/ACE/ace/Service_Repository.cpp
@@ -151,12 +151,14 @@ ACE_Service_Repository::fini (void)
 
       // Do not be tempted to use the prefix decrement operator.  We
       // need to use the postfix decrement operator in this case since
-      // the index is unsigned.
+      // the index is unsigned and may wrap around the 0
       for (size_t i = this->current_size_; i-- != 0; )
         {
           ACE_Service_Type *s =
             const_cast<ACE_Service_Type *> (this->service_vector_[i]);
 
+          if (s == 0) continue; // skip any gaps
+
 #ifndef ACE_NLOGGING
           if (ACE::debug ())
               ACE_DEBUG ((LM_DEBUG,
@@ -202,20 +204,22 @@ ACE_Service_Repository::close (void)
 
       // Do not be tempted to use the prefix decrement operator.  We
       // need to use the postfix decrement operator in this case since
-      // the index is unsigned.
+      // the index is unsigned and may wrap around the 0.
       for (size_t i = this->current_size_; i-- != 0; )
         {
+          ACE_Service_Type *s =
+            const_cast<ACE_Service_Type *> (this->service_vector_[i]);
+
+          if (s == 0) continue;
 
 #ifndef ACE_NLOGGING
           if(ACE::debug ())
             ACE_DEBUG ((LM_DEBUG,
                         ACE_TEXT ("(%P|%t) SR::close, this=%@, delete so[%d]=%@ (%s)\n"),
                         this, i,
-                        this->service_vector_[i],
-                        this->service_vector_[i]->name ()));
+                        s, s->name ()));
 #endif
 
-          ACE_Service_Type *s = const_cast<ACE_Service_Type *> (this->service_vector_[i]);
           --this->current_size_;
           delete s;
         }
@@ -255,9 +259,12 @@ ACE_Service_Repository::find_i (const ACE_TCHAR name[],
   size_t i;
 
   for (i = 0; i < this->current_size_; i++)
-    if (ACE_OS::strcmp (name,
-                        this->service_vector_[i]->name ()) == 0)
-      break;
+    {
+      if (this->service_vector_[i] != 0 // skip any empty slots
+          && ACE_OS::strcmp (name,
+                             this->service_vector_[i]->name ()) == 0)
+        break;
+    }
 
   if (i < this->current_size_)
     {
@@ -300,6 +307,8 @@ ACE_Service_Repository::relocate_i (size_t begin,
       ACE_Service_Type *type =
         const_cast<ACE_Service_Type *> (this->service_vector_[i]);
 
+      if (type == 0) continue; // skip any gaps
+
       ACE_SHLIB_HANDLE old_handle =  type->dll ().get_handle (0);
       if (old_handle == ACE_SHLIB_INVALID_HANDLE && new_handle != old_handle)
       {
@@ -334,10 +343,9 @@ ACE_Service_Repository::find (const ACE_TCHAR name[],
 
 
 // Insert the ACE_Service_Type SR into the repository.  Note that
-// services may be inserted either resumed or suspended. Using same name
-// as in an existing service causes the delete () to be called for the old one,
-// i.e. make sure @code sr is allocated on the heap!
-
+// services may be inserted either resumed or suspended. Using same
+// name as in an existing service causes the delete () to be called
+// for the old one, i.e. make sure @code sr is allocated on the heap!
 int
 ACE_Service_Repository::insert (const ACE_Service_Type *sr)
 {
@@ -347,34 +355,37 @@ ACE_Service_Repository::insert (const ACE_Service_Type *sr)
   ACE_Service_Type *s = 0;
   size_t i = 0;
 
+  // Establish scope for the service vector manipulation (i.e. locking)
   {
     // @TODO: Do we need a recursive mutex here?
     ACE_MT (ACE_GUARD_RETURN (ACE_Recursive_Thread_Mutex, ace_mon, this->lock_, -1));
 
     // Check to see if this is a duplicate.
     for (i = 0; i < this->current_size_; i++)
-    {
-        if (ACE_OS::strcmp (sr->name (),
-                          this->service_vector_[i]->name ()) == 0)
-        {
+      {
+        if (this->service_vector_[i] != 0 // skip any gaps
+            && ACE_OS::strcmp (sr->name (),
+                               this->service_vector_[i]->name ()) == 0)
+          {
             // Replacing an existing entry
             return_value = 0;
+
             // Check for self-assignment...
             if (sr != this->service_vector_[i])
-            {
+              {
                 s = const_cast<ACE_Service_Type *> (this->service_vector_[i]);
                 this->service_vector_[i] = sr;
-            }
+              }
 
             break;
-        }
-    }
+          }
+      }
 
     // Adding an entry.
     if (i >= this->total_size_)
-    {
+      {
         return_value = -1; // no space left
-    }
+      }
     else if (s == 0)
       {
         this->service_vector_[i] = sr;
@@ -382,13 +393,13 @@ ACE_Service_Repository::insert (const ACE_Service_Type *sr)
         return_value = 0;
 
 #ifndef ACE_NLOGGING
-    if (ACE::debug ())
-      ACE_DEBUG ((LM_DEBUG,
-                  ACE_TEXT ("ACE (%P|%t) SR::insert - repo=%@ [%d] (%d),")
-                  ACE_TEXT (" name=%s (new), type=%@, object=%@, active=%d\n"),
-                  this, i, this->total_size_, sr->name(), sr->type (),
-                  (sr->type () != 0) ? sr->type ()->object () : 0,
-                  sr->active ()));
+        if (ACE::debug ())
+          ACE_DEBUG ((LM_DEBUG,
+                      ACE_TEXT ("ACE (%P|%t) SR::insert - repo=%@ [%d] (%d),")
+                      ACE_TEXT (" name=%s (new), type=%@, object=%@, active=%d\n"),
+                      this, i, this->total_size_, sr->name(), sr->type (),
+                      (sr->type () != 0) ? sr->type ()->object () : 0,
+                      sr->active ()));
 #endif
       }
   }
@@ -415,7 +426,6 @@ ACE_Service_Repository::insert (const ACE_Service_Type *sr)
 }
 
 // Re-resume a service that was previously suspended.
-
 int
 ACE_Service_Repository::resume (const ACE_TCHAR name[],
                                 const ACE_Service_Type **srp)
@@ -481,19 +491,16 @@ ACE_Service_Repository::remove (const ACE_TCHAR name[], ACE_Service_Type **ps)
  *
  * Since the order of services in the Respository matters, we can't
  * simply overwrite the entry being deleted with the last and
- * decrement the <current_size> by 1 - we must "pack" the array.  A
- * good example of why the order matters is a dynamic service, in
- * whose DLL there is at least one static service. In order to prevent
- * SEGV during finalization, those static services must be finalized
- * _before_the dynamic service that owns them. Otherwice the TEXT
- * segment, containing the code for the static service's desructor may
- * be unloaded with the DLL.
+ * decrement the <current_size> by 1.  A good example of why the order
+ * matters is a dynamic service, in whose DLL there is at least one
+ * static service. In order to prevent SEGV during finalization, those
+ * static services must be finalized _before_the dynamic service that
+ * owns them. Otherwice the TEXT segment, containing the code for the
+ * static service's desructor may be unloaded with the DLL.
  *
- * @note: (IJ) The above is not entirely true, since the introduction
- * of the ACE_Service_Dynamic_Guard, which fixes-up any stray static
- * services to hold a reference to the DLL. This allows out-of order
- * removals and perhaps allows to skip packing the repo. I left it in
- * because a packed repo is a lot easier to debug.
+ * Neither can we "pack" the array because this may happen inside the
+ * scope of a Service_Dynamic_Guard, which caches an index where
+ * loading of a DLL started in order to relocate dependent services.
  */
 int
 ACE_Service_Repository::remove_i (const ACE_TCHAR name[], ACE_Service_Type **ps)
@@ -505,11 +512,7 @@ ACE_Service_Repository::remove_i (const ACE_TCHAR name[], ACE_Service_Type **ps)
   // We may need the old ptr - to be delete outside the lock!
   *ps = const_cast<ACE_Service_Type *> (this->service_vector_[i]);
 
-  // Pack the array
-  --this->current_size_;
-  for (size_t j = i; j < this->current_size_; j++)
-    this->service_vector_[j] = this->service_vector_[j+1];
-
+  this->service_vector_[i] = 0; // simply leave a gap
   return 0;
 }
 
@@ -574,8 +577,11 @@ bool
 ACE_Service_Repository_Iterator::valid (void) const
 {
   ACE_TRACE ("ACE_Service_Repository_Iterator::valid");
-  return (this->ignore_suspended_ == 0
-          || this->svc_rep_.service_vector_[this->next_]->active ());
+  if (this->ignore_suspended_ == 0)
+    return (this->svc_rep_.service_vector_[this->next_] != 0); // skip over gaps
+
+  return (this->svc_rep_.service_vector_[this->next_] != 0
+          && this->svc_rep_.service_vector_[this->next_]->active ());
 }
 
 ACE_END_VERSIONED_NAMESPACE_DECL