fix memory leak(#151) #168
Conversation
Add codes to free context to fix the memory leak discussed at #151. - Replace uv_poll_stop with uv_close to unref the poll_handler - Add mutexs to PollStop to guard asynchronous invocation by the end of flush operation and synchronous invocation by close operation - Call free(context) to free memory of struct context - Execute above free operation when they are called twice using mutexes to guarantee the memory is to be freed at last. Because wrap_pointer_cb called by GC and the latter part of uv_close (and close_cb) will be called asynchronously and disorderly.
agreed. also still reviewing your fix this week |
|
also proof of memory leak fixes, maybe some test code and a graph that shows the memory usage before+after the patch |
IMO the test failed partially b/c subscription shutdown is known to be brittle
yes, +1, although it's intersting that libnanomsg uses global lock operations
best to observe that kind of thing against long running processes, so i'll update w/ my findings later this week |
sure, but i'd rather have C11 than C++11... C language should be plenty to do whatever we want. also i have nothing against C++11 |
|
key takeaways are the leaks are from missing free/deletes to match |
- apply clang-format following instruction at README.md - move uv_mutex_unlock and info.GetReturnValue().Set() to the end of PollStop because they are duplicated in both if and else scopes - move bool variables to the end of struct and remove typedef - remove all inline decorators and move functions in the header to the source file.
|
I have updated the code with comments by @nickdesaulniers. Please review them again. In addition to the commit log:
|
| @@ -113,7 +113,7 @@ NAN_METHOD(Send) { | |||
|
|
|||
| void fcb(char *data, void *hint) { | |||
| nn_freemsg(data); | |||
| (void) hint; | |||
| (void)hint; | |||
| } | |||
There was a problem hiding this comment.
void fcb(char *data, void *) {
nn_freemsg(data);
}
|
|
||
| // this function is called asynchronously at destructor when garbage collection | ||
| // is executed | ||
| static void wrap_pointer_cb(char *data, void *hint) { |
There was a problem hiding this comment.
static void wrap_pointer_cb(char *data, void *) {
|
|
||
| template <typename Type> | ||
| static Type UnwrapPointer(v8::Local<v8::Value> buffer) { | ||
| return reinterpret_cast<Type>(UnwrapPointer(buffer)); |
| // this function is called asynchronously after uv_close function call | ||
| void close_cb(uv_handle_t *handle) { | ||
| nanomsg_socket_s *context = | ||
| reinterpret_cast<nanomsg_socket_s *>(handle->data); |
| uv_mutex_t close_mutex; | ||
| uv_mutex_t free_mutex; | ||
| bool close_called; | ||
| bool free_called; |
There was a problem hiding this comment.
please change the name to nanomsg_poll_ctx and refer to it throughout the code as struct nanomsg_poll_ctx.
| } | ||
|
|
||
| // this function is called asynchronously after uv_close function call | ||
| void close_cb(uv_handle_t *handle) { |
There was a problem hiding this comment.
static void close_cb(uv_handle_t *handle) {
|
prefer #169 thank you for your help and interest in fixing this issue. |
#151
I could fix this memory leak. Please review and improve my code if necessary(Especially I think the timing to free the memory is unusual).
With this fix, I make sure the memory leak is disappeared using the test tool by @deepakprabhakara as follows:
result of original code
result of fixed code
Add codes to free context to fix the memory leak discussed at #151.
of flush operation and synchronous invocation by close operation
to guarantee the memory is to be freed at last. Because
wrap_pointer_cb called by GC and the latter part of uv_close
(and close_cb) will be called asynchronously and disorderly.