-
September 26th, 2011, 05:20 AM
#16
Re: Segmentation Fault ??
I have not taken a really good look at your code, but just a warning: the CSocket destructor should be declared virtual. Declaring the destructor of its derived class as virtual is not enough.
-
September 26th, 2011, 05:39 AM
#17
Re: Segmentation Fault ??
One other thing.... how does your program know that the object will need to be deleted in precisely 4 minutes' time? Generally speaking, you delete an object when you no longer need it and you don't normally know this, four minutes in advance.
The 4 minute delay suggests to me that some other process is somehow still using the object and you're just making a guess about how long it might need it for.
Or to put it another way, try re-instating the virtual function from yesterday but increase 240 to 360. My guess is that this will also make the problem magically go away. So will a lot of other arbitrary changes. You need to look for the kind of thing that Laserlight suggested. It'll end up being something like that or a pointer corruption problem.
It's a great shame that you can't reproduce the problem in VC++ because it's much better for debugging this kind of problem than gdb.
"A problem well stated is a problem half solved.” - Charles F. Kettering
-
September 26th, 2011, 05:42 AM
#18
Re: Segmentation Fault ??
m_ClientsDeleting.push_back(client);
i = m_ClientsDeleting.erase( i );
Are these two function calls occurring in the same thread?
If they are not, then this will almost certainly be a source of memory corruption.
"It doesn't matter how beautiful your theory is, it doesn't matter how smart you are. If it doesn't agree with experiment, it's wrong."
Richard P. Feynman
-
September 26th, 2011, 07:00 AM
#19
Re: Segmentation Fault ??
yes, i do not insert elements into vectors or containers of alien threads.
the Clients* instance is used for the MYSQL login process of the clients.
.erase( ) is called on the same thread. all i share with other threads is boolean variables.
so the only thread that tries to access it is the mysql thread with the mysql login function.
although the function is safe because
i get the std::string m_Name of the client and copy it so i can use it in my queries.
then i try to access the pointer at the end of the query so i can set the :
Clients::m_Identified flag to true.
ii use the CBoolExpr( ) function for that.meaning that the object is used at the start of the function before the queries (thus the blocking of code)
and at the end of the function after the queries.
Originally Posted by laserlight
I have not taken a really good look at your code, but just a warning: the CSocket destructor should be declared virtual. Declaring the destructor of its derived class as virtual is not enough.
thanks, didn't notice that before.
---EDIT
so the server is still running from yesterday, but i'm 99% sure you guys are right about memory corruption in my code.
although i don't precisely know why.
here's the destructor of the Clients class
Code:
Clients :: ~Clients( )
{
delete m_Socket;
m_Socket = NULL;
while(!m_CPackets.empty() )
{
delete m_CPackets.front();
m_Packets.pop();
}
}
Yes you are right. I totally forget a C in the m_Packets.pop( )
seems we got some major memory leaks
--EDIT
and this is how it should be
Code:
Clients :: ~Clients( )
{
delete m_Socket;
m_Socket = NULL;
while(!m_CPackets.empty() )
{
delete m_CPackets.front();
m_CPackets.pop();
}
while(!m_Packets.empty() )
{
m_Packets.pop();
}
}
constructor :
Code:
class Clients
{
public:
CTCPSocket *m_Socket;
CChat *m_Chat;
CPDC *m_PDC;
string m_Name;
uint32_t m_Id;
uint32_t m_LastSend;
uint16_t m_Version;
string m_ChatRoom;
queue<BYTEARRAY> m_Packets;
queue<CCPacket*> m_CPackets;
.... // more vars
Clients( CTCPSocket *socket, CPDC *pdc );
virtual ~Clients( );
};
Last edited by JacobNax; September 26th, 2011 at 07:15 AM.
-
September 26th, 2011, 10:08 AM
#20
Re: Segmentation Fault ??
Originally Posted by JacobNax
heres some clue about the life-cycle of the Clients* object
1) The code you posted doesn't show where you are setting pointers to NULL so that checking for NULL will work correctly. Deleting an object does not set an object to NULL.
2) If your code is holding onto iterators or pointing to entries in the vector whenever the vector is resized (in this case grown), then those iterators or pointers can become invalidated here:
Code:
m_Clients.push_back(client);
3) This loop could be rewritten using algorithms.
Code:
for(vector<Clients*> :: iterator i = m_Clients.begin( ); i != m_Clients.end( );)
{
if( (*i)->m_Socket && (*i)->m_Socket->GetConnected( ) && !(*i)->m_Socket->HasError( ) )
{
(*i)->m_Socket->SetFD(&fd, &send_fd, &nfds);
nfds++;
i++;
}
else
{
cout << "A client connection was lost." << endl;
CleanUpUser((*i));
i = m_Clients.erase( i );
}
}
If the goal is to erase all closed connections in the loop, and to call SetFD on the good connections, that probably could have been accomplished this way:
Code:
#include <algorithm>
//...
bool IsGoodConnection(Client* pClient)
{
return pClient->m_Socket &&
pClient->m_Socket->GetConnected( ) &&
!pClient->>m_Socket->HasError( );
}
struct FDSetter
{
int fd;
int send_fd;
int nfds;
FDSetter() : nfds(1) {}
void operator()(Client* pClient)
{
pClient->m_Socket->SetFD(&fd, &send_fd, &nfds);
++nfds;
}
};
void SetFD(Client *pClient)
{
// good connections on left of partition, bad/closed connections on right of partition
vector<Clients*>::iterator partition = std::stable_partition(m_Clients.begin(), m_Clients.end(), IsGoodConnection);
// get the number of good entries (left of partition)
nfds = std::distance(m_Clients.begin(), partition);
// for each good connection, call SetFD
std::for_each(m_Clients.begin(), partition, FDSetter());
// erase the bad connections by calling CleanUpUser and then erase them from the vector
std::for_each(partition, m_Clients.end(), CleanUpUser);
m_Clients.erase(partition, m_Clients.end();
If what you wanted to do was to get rid of the closed connections, erase them from the vector, and keep/setup the good connections, then partitioning them first so that they are grouped accordingly could have been done. Note the use of stable_partition so as to maintain the order of the connections in the vector.
Note that there are no loops. I did not know what to set fd and send_fd in the FDSetter class, since you didn't specify what they were or how they are used in the SetFD call. But the point is that writing loops as you've done can be error prone.
Usage of algorithms guarantees that the code will work, provided that you call the correct algorithm functions with the correct arguments. In other words, there is no need for me to check the code above for iterator bugs or some other issue that could easily happen when attempting to write your own loops. It is also self-documenting, as any competent C++ programmer knows what those algorithm functions do. When given hand-coded loops as you've been doing, it may take several looks throught it to a) knows whart it does, and b) be assured it works correctly.
The loop writing, erasing, making sure that iterators are seated properly, etc. -- leave that work to the algorithms.
Regards,
Paul McKenzie
Last edited by Paul McKenzie; September 26th, 2011 at 10:15 AM.
-
September 26th, 2011, 10:29 AM
#21
Re: Segmentation Fault ??
thanks Paul
your version looks neat and well written
i haven't studied much the algorithms, just how the sort() and shuffling works.
Tags for this Thread
Posting Permissions
- You may not post new threads
- You may not post replies
- You may not post attachments
- You may not edit your posts
-
Forum Rules
|
Click Here to Expand Forum to Full Width
|