Changeset 2070 for code/branches/objecthierarchy/src/network/packet

Timestamp:

Oct 31, 2008, 12:05:12 AM (16 years ago)

Author:

rgrieder

Message:

Properly took care of network::packet::Packet destruction. It depends very much whether the the data was allocated by ENet or by ourselves.

Location:

code/branches/objecthierarchy/src/network/packet

Files:

• Gamestate.cc (modified) (1 diff)
• Packet.cc (modified) (6 diffs)
• Packet.h (modified) (1 diff)

code/branches/objecthierarchy/src/network/packet/Gamestate.cc

@@ -286 +286 @@
   //copy over the header
   *GAMESTATE_HEADER(ndata) = *HEADER;
-  //delete old (compressed data)
-  delete[] data_;
+
+  if (this->bDataENetAllocated_){
+    // Memory was allocated by ENet. --> We let it be since enet_packet_destroy will
+    // deallocated it anyway. So data and packet stay together.
+    this->bDataENetAllocated_ = false;
+  }
+  else{
+    // We allocated the memory in the first place (unlikely). So we destroy the old data
+    // and overwrite it with the new decompressed data.
+    delete[] this->data_;
+  }
+
   //set new pointers
   data_ = ndata;

code/branches/objecthierarchy/src/network/packet/Packet.cc

@@ -61 +61 @@
   data_=0;
   enetPacket_=0;
+  bDataENetAllocated_ = false;
 }
 
@@ -73 +74 @@
   data_=data;
   enetPacket_=0;
+  bDataENetAllocated_ = false;
 }
 
@@ -86 +88 @@
   }else
     data_=0;
-}
-
+  bDataENetAllocated_ = p.bDataENetAllocated_;
+}
+
+/**
+@brief
+    Destroys a packet completely.
+    
+    That also means destroying the ENetPacket if one exists. There 
+*/
 Packet::~Packet(){
-  if(enetPacket_){
-    assert(enetPacket_->freeCallback==0);
+  // Deallocate data_ memory if necessary.
+  if (this->bDataENetAllocated_){
+    // In this case ENet allocated data_ and will destroy it.
+  }
+  else if (this->data_) {
+    // This destructor was probably called as a consequence to ENet executing our callback.
+    // It simply serves us to be able to deallocate the packet content (data_) ourselves since
+    // we have created it in the first place.
+    delete[] this->data_;
+  }
+
+  // Destroy the ENetPacket if necessary.
+  // Note: For the case ENet used the callback to destroy the packet, we have already set
+  // enetPacket_ to NULL to avoid destroying it again.
+  if (this->enetPacket_){
+    // enetPacket_->data gets destroyed too by ENet if it was allocated by it.
     enet_packet_destroy(enetPacket_);
   }
-  if(data_)
-    delete[] data_;
 }
 
@@ -107 +128 @@
       return false;
     }
+    // We deliver ENet the data address so that it doesn't memcpy everything again.
+    // --> We have to delete data_ ourselves!
     enetPacket_ = enet_packet_create(getData(), getSize(), getFlags());
     enetPacket_->freeCallback = &Packet::deletePacket;
-//     enetPacket_->freeCallback = &blub;
+    // Add the packet to a global list so we can access it again once enet calls our
+    // deletePacket method. We can of course only give a one argument function to the ENet C library.
     packetMap_[enetPacket_] = this;
   }
@@ -127 +151 @@
   }
 #endif
-  ENetPacket *temp = enetPacket_;
-  enetPacket_ = 0; // otherwise we have a double free because enet already handles the deallocation of the packet
-  network::Host::addPacket( temp, clientID_);
+//  ENetPacket *temp = enetPacket_;
+//  enetPacket_ = 0; // otherwise we have a double free because enet already handles the deallocation of the packet
+  network::Host::addPacket( enetPacket_, clientID_);
   return true;
 }
@@ -170 +194 @@
       break;
   }
+
+  // Data was created by ENet
+  p->bDataENetAllocated_ = true;
+
   return p;
 }
 
-void Packet::deletePacket(ENetPacket *packet){
-  assert(packetMap_[packet]);
-  assert(packetMap_[packet]->enetPacket_==0);
-  delete packetMap_[packet];
+/**
+@brief
+    ENet calls this method whenever it wants to destroy a packet that contains
+    data we allocated ourselves.
+*/
+void Packet::deletePacket(ENetPacket *enetPacket){
+  // Get our Packet from a gloabal map with all Packets created in the send() method of Packet.
+  std::map<ENetPacket*, Packet*>::iterator it = packetMap_.find(enetPacket);
+  assert(it != packetMap_.end());
+  // Make sure we don't delete it again in the destructor
+  it->second->enetPacket_ = 0;
+  delete it->second;
+  packetMap_.erase(it);
 }
 

code/branches/objecthierarchy/src/network/packet/Packet.h

@@ -84 +84 @@
     unsigned int clientID_;
     ENUM::Direction packetDirection_;
+    /** Pointer to the data. Be careful when deleting it because it might
+        point to a location that was allocated by ENet.
+        See bDataENetAllocated_ */
     uint8_t *data_;
+    /** Tells whether data_ was allocated by ENet or ourselves.
+        data_ might no correlate with enetPacket_->data. */
+    bool bDataENetAllocated_;
   private:
     static std::map<ENetPacket *, Packet *> packetMap_;