Fix memleak in signal package #1114
Conversation
|
|
||
| str := C.CString(string(data)) | ||
| C.StatusServiceSignalEvent(str) | ||
| C.free(unsafe.Pointer(str)) |
There was a problem hiding this comment.
Have you verified that we don't cause any invalid memory reads on Android and iOS? For example, in Android, C.StatusServiceSignalEvent we do if (javaJsonEvent != NULL) (*env)->DeleteLocalRef(env, javaJsonEvent);.
There was a problem hiding this comment.
@adambabik no, I haven't. But as I understand, CCstring always make a copy and responsibility to free its memory is on the caller.
javaJsonEvent is a separate chunk of memory so it's ok that it is deleted there:
jstring javaJsonEvent = NULL;
if (jsonEvent != NULL) {
javaJsonEvent = (*env)->NewStringUTF(env, jsonEvent);
}
|
I'll test it on a real server built from the branch before merging. |
|
Update after running fix for ~20 hours. Memory increased from 17MB to 25MB (vs 17MB to 109MB for the same period before the fix).
|
We were investigating memory leak in a server instance of statusd, and here is a brief explanation of findings:
#1067 (comment)
TLDR:
a) we call signals even when running as a server
b) we historically never free memory allocated by
C.CString.I believe this fix should stop memory from growing (or at least notable decrease growth).
Closes #1067