Hi Max,
I hope I can steal some of your time and can ask a question. :-)
Question: Why did you choose different cleanup tactics for the different client socket objects?
As far as I understood, the socket objects will be (as usual) assigned with new(), stored on a list. When the correspoding socket is closed, the socket object is removed from the list and the object destroyed with delete(). This last step, the remove tactics, seems to be different for different client sockets.
MGnuDirector is the listening server socket.
a) MGnuSock is an new incoming client socket (stored in m_SockList), will then be turned into:
b) MGnuNode a Gnutella connection after handshake (in m_NodeList )
c) MGnuUpload a upload, from a HTTP GET request (in m_UploadList)
d) MGnuDownload a download, from a GIV push request (in m_DownloadList)
To a) Socket is destroyed from server in MGnuDirector::OnTimer()
when MGnuSock object signs m_bDestroy==TRUE. Server tests this every second.
To b) Socket is destroyed from server in MGnuDirector::RemoveNode().
Which is called from the socket object itself, e.g. in OnClose() or whenever the socket closes itself. Cool, is there something critical which has to be avoided when a object deletes itself, to avoid runtime race conditions or crashs? It might only work because you run all socket stuff from one thread?
To c) Socket is destroyed from server in MGnuDirector::OnTimer()
when MGnuDownload signs m_nSecInactive>30. Server tests this every second. Also socket calls MGnuDirector::TransferMessage, which is obsolete?
To d) Socket is destroyed from server in MGnuDirector::OnTimer()
when MGnuUpload signs m_nStatus==TRANSFER_CLOSED || m_nStatus==TRANSFER_COMPLETED. Server tests this every second. Also socket calls MGnuDirector::TransferMessage, which is obsolete?
Suggestion:
Now.... how about doing it the same way for removing dead sockets? For example implement a RemoveNode() method for all socket types.... or using a timer based solution for all socket types? Just a suggestion.
Misc:
Some stuff I recognized or don't understand.
- void MGnuSock::OnReceive()
DWORD dwBuffLength = Receive(m_pBuff, 32768);
//shouldn't we collect bytes and split lines on '\n'?
//you could use sizeof(m_pBuff) instead 32768
m_bDestroy = true;
//shouldn't this be set at the end of the method, not at the beginning?
//or maybe better at the end of ForceDisconnect() and Close()?
- void MGnuDirector::OnTimer()
if(pSock->m_nSecsAlive > 15 || pSock->m_bDestroy)
// 15 sec will never reached because m_bDestroy becomes TRUE first?
if (pDown->m_nSecInactive>30)
// hmm, m_status==TRANSFER_CLOSED is never evaluated?
- Server socket class destructor should clean up also m_SockList?
- All socket classes destructors should do this: if (m_hSocket != INVALID_SOCKET) Close();
- Closing sockets should be
more friendly, a ForceDisconnect should only used if necesarry.
Greets, Moak