Big memory leak, on client disconnect, in server that is started with the runInBackground argument set to TRUE

[JorisHansMeijer]

When starting the server with the runInBackground argument set to TRUE a big memory leak happens each time a client disconnects. This is because the client_thread is not joined after detach. I fixed this by adding pthread_detach(cl->client_thread); at the end of the clientInput function in main.c (just before the return).

Because no the thread doesn't need to be joined I have also removed the join that was there in rfbShutdownServer. This join will only join the clients that are active at the time of stopping the server (not the clients that have disconnected before rfbShutdownServer was called).

[bk138]

Thanks for finding out! Could you be so kind and file a pull request? This way, we can also have proper author credits :-)

[JorisHansMeijer]

Done

[bk138]

list of things to check for @bk138

• win32 threads detach, how would it work
  • look for function and add to threading.h -> find
  • or maybe use https://tinycthread.github.io/ or https://github.com/jtsiomb/c11threads ?
• test detach case for "client disconnects"
• test detach case for "server shuts down with active clients"
• think about pipe-notify machinery -> is for breaking the select() wait in client thread in close event

[bk138]

This needs some more love as 8560a5a alone caused bk138/droidVNC-NG#163

• re-apply 8560a5a ?
• rework rfbShutdownServer
• rework rfbCleanupServer
• test test test (with additional delays injected)

[JorisHansMeijer]

Is it correct that the problem happens a shutting down the server? Looking at the code again I can see that pthread_join would wait for the client thread to finish execution. Removing pthread join (as detached trhead's can't be joined) would also remove the waiting for the client to finish code. So the solution seems to be to just wait for the client to finish instead of the previous pthread_join call.

[bk138]

Exactly. How would we best wait for a detached thread to finish?

[JorisHansMeijer]

Maybe the best way would be using pthread_kill, it can be used in a simulation mode were it just test if the signal could be send. In this way it is possible to know if a thread is still running. Example (untested):
while(pthread_kill(currentCl->client_thread, 0) != ESRCH){
sleep some ms..
}

[bk138]

Hmm, busy sleep sounds like we can do better? Maybe a condition variable or the like? (disclaimer: I'm not soo familiar with pthreads...)

[JorisHansMeijer]

Some more digging reveals that using pthread_kill on a canceled thread is not a good way to go (in linux I believe it is fine, but not for all pthread implementations out there). Maybe a better way to go is keep track of the thread termination in the client object (add running variable, set it at thread start, clear it at thread detach) and wait for that to clear in the termination function.

The time between stopping the server and waiting for the threads to finish should not be long so a loop with a short sleep would not be that much of a issue I think (just a couple of ms using nanosleep on linux and something similar on windows).

As a alternative it is possible to NOT detach the threads that are still running at server termination (conditional detach). Than the original join can be put back. However this might cause problems with clients that are just in the process of detaching when the server is stopped.