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

General answer: I haven't chosen those tactics, I've inherited it from Gnucleus with other bits of crap here and there and trying to clean up the code without breaking everything since day 1. So, IMHO the question should be addressed to the developers of Gnucleus

but, ok step by step:
a) MGnuSocket is a bit special type of socket, it's an incomming connection with yet undefined type. That is the other side did not initiated handshake yet. There is certain timeout while it makes sence to wait for a handshake, and when it's over the socket has to go. At the moment the implementation is ugly but it improves (v0.4 is somewhere on the way)

b) Historical reason. probably have to review it.

c,d) In principle it seems to make sence to remove all sockets only here, that is in
OnTimer()

see the begining of the message. Btw, it's gone in the current version (will be on CVS probably on Thursday)

this is right, but the current implementation only will produce memory leaks on application exit, which is not that important

something like that is already done in the MAsyncSocket's destructor

In general -- agree. Say "Shutdown" and than after few seconds "Close".

Thanks for the comments,
Guess on day I'll have time to complete and clean up all this mess...

--Max

Hi Max,
it's not really important, but I'm curious what is the purpose of this Mutex in common.cpp:

time_t xtime()
{
MLock lock(g_LibcMutex);
return time(NULL);
}

Thx, Moak

Hi Moak,

I had VERY strange crashes, and because time() function is said to be non-thread safe I've decided tho protect it with the mutex. The real reason for those crashes was different, but xtime() stayed...

oki thx!

Hi Max,

mainly about DNS and asynchronous socket class.... some ideas for a CAsyncSocket redesign:

- Okay we work usually IP based (just to get a starting point)
- Run a second thread which will resolve all pending DNS resolutions and store them in an array, big enough to cache all needed hostnames of an application. Size of this internal lookup table can be adjusted by user.
- Two new methods BOOL Resolve(unsigned long *ipaddr) and BOOL Resolve(char* hostname). Which will query the array (multithreading safe) and eighter return the result... or FALSE and error result ADDRESS_NOT_AVAILABLE (can't be resolved) or ADDRESS_RESOLVING. In any case, the methods return immadiately and do not block!
- User can query again and again for an address, until the address is resolved (in case of a FALSE return he has to continue to work with IP).
- New virtual override OnResolve() which will notify the socket when a name server lookup was finished. This event might be a sucessfull finished lookup or telling that address can't be resolved. OnResolve() will called once after a succesfull/unsucessfull lookup... or more technically whenever a Resolve() call will change the internal lookup table.

Other stuff:

- new virtual override OnReceiveLine(), which will only called when a full line was received. Simply override OnReceive with a call to ReceiveLine(), it will do line buffering in a per socket based buffer.
- add a status to each socket which can be queried with int GetStatus(), here what I use currently:

enum {
SOCKET_DEAD = 0, //socket is dead (delete socket)
SOCKET_DISCONNECTING, //socket is performing disconnect
SOCKET_UNCONNECTED, //socket is not connected
SOCKET_CONNECTING, //socket is connecting
SOCKET_CONNECTED, //socket is connected
SOCKET_LISTENING, //socket is listening (not available for incoming "client" sockets)
}
;

Some things simplyfied (e.g. method parameters and return values). I have a small testing code which is only a few lines... don't ask me for full code before end of summer.

Greets, Moak

PS: Raphael, of course you would do better.

Max,

was there a reason why you choosen CString over std::string? *curious*

Btw, maybe you want to add a "parachute" to your TString::format() code: if there is '%n' in the format string (fmt) better don't evaluate the expression. Since you can't verify the argument types feeded on runtime, this could be exploited (by accident). Nobody really needs unsafe %n. Hope it helps.

Moak

Yep. The std::string provided with g++ 2.95 is thread unsafe, but it only can be demonstrated on a SMP box (which I do have). So I had to write my own string. The other reason why my string is somewhat similar to MFC's CString is because I'm comming from the win32 world.

Good idea, however, I never use %n. But I did get few crashes (not with mutella) because of typing %n instead of %d.

Cheers
--Max

It was me in the previous post, I just did not use this PC for a while and cookies have expired.

By the way, it's nice to see you back, Moak. Well, it might be that you were back all the time and it was me who was out. Well, I am still... Moving next Wednesday over 500 km. Pain in the back...

Yeah, before I forget, send me your socket class when you are kinda done with it -- I'm interested to have a look. Hope I do have time. Async DNS is a good idea, it is something we are really need for mutella. I'll probably do something about it sometimes soon. Yeah, opinions... Line parsing IMHO should be optional, may be controlled by switches or something. Built-in status is very appropriate. This is it so far. Sorry. Busy ripping the carpets off. :-)

Regards,
Max