Skip to content

Commit

Permalink
OSX: Fix SDL_Thread leaks when updating server browser
Browse files Browse the repository at this point in the history 
0.8kb leak when updating server browser, and many other "one off"
threads that are created.

Leak: 0x7fc32be28720  size=816  zone: DefaultMallocZone_0x10849e000
	0x0021d000 0x00007000 0x0021d000 0x00007000 	..!..p....!..p..
	0x00000000 0x00000002 0x00000000 0x00000000 	................
	0x00000000 0x00000000 0x00000000 0x00000000 	................
	0x00000000 0x00000000 0x00000000 0x00000000 	................
	0x00000000 0x00000000 0x00000000 0x00000000 	................
	0x00000000 0x00000000 0x00000000 0x00000000 	................
	0x00000000 0x00000000 0x00000000 0x00000000 	................
	0x00000000 0x00000000 0x00000000 0x00000000 	................
	...
	Call stack: [thread 0x700000081000]:
        |  thread_start
        |  _pthread_body
        |  _pthread_body
        |  RunThread
        |  SDL_RunThread
        |  GetServerPingsAndInfosProc EX_browser_net.c:574
        |  PingHosts EX_browser_ping.c:785
        |  Sys_CreateThread sys_posix.c:358
        |  SDL_CreateThread_REAL
        |  malloc
        |  malloc_zone_malloc

The result of SDL_CreateThread was not being used, but needed to
be freed. Documentation says to either call WaitThread (join) to
wait for the thread to complete, or DetachThread (detach) to have
the thread clean itself up when it completes. The intent here
looks to be let the thread clean itself up, so call DetachThread.
  • Loading branch information
@JosephPecoraro @meag
JosephPecoraro authored and meag committed Jul 9, 2016
1 parent 46859c7 commit 6926046
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion sys_posix.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,8 @@ void Sys_MakeCodeWriteable (unsigned long startaddr, unsigned long length) {

int Sys_CreateThread(DWORD WINAPI (*func)(void *), void *param)
{
SDL_CreateThread((SDL_ThreadFunction)func, NULL, param);
SDL_Thread *thread = SDL_CreateThread((SDL_ThreadFunction)func, NULL, param);
SDL_DetachThread(thread);
return 1;
}

Expand Down

4 comments on commit 6926046

@jite
Copy link
Contributor

@jite jite commented on 6926046 Jul 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not really a pretty solution. A better solution would be to return a thread id/handle (if possible, SDL_Thread might work if all systems use this) and let caller decide.

Alternatively, create a new func that doesn't detach and do as described.

@meag
Copy link
Contributor

@meag meag commented on 6926046 Jul 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It solves the immediate issue of the memory leak, but Windows is still not closing handles. I've raised an issue for that as it will need Wait/CloseHandle, can look this again during work on that.

The only place this gets called at the moment is in the server browser (which needs looked at anyway) and uploading match logs (ditto). In both places they expect "fire and forget", so SDL_DetachThread behaviour seems valid.

@jite
Copy link
Contributor

@jite jite commented on 6926046 Jul 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep detaching the thread is the right thing to do. Though the Sys_* API is (or was) basically on the same level as the SDL_* API, meaning that adding the DetachThread to Sys_CreateThread() would basically be the same thing as adding it within SDL_CreateThread which isn't really proper :)

What I meant was that there should be two Sys_ functions (if needed, if all platforms use SDL then there's perhaps no need for Sys_CreateThread at all, otherwise), Sys_CreateThread() and Sys_CreateDetachedThread() since we're on API level.

@jite
Copy link
Contributor

@jite jite commented on 6926046 Jul 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows implementation should probably be moved to SDL too, however the Windows side is leaking cause it lacks a CloseHandle() call.

Please sign in to comment.