Fix memory leak. Not all tasks are deleted in thread_d dtor. #130
Conversation
| current->cur_task = 0; | ||
| next->_set_active_context(0); | ||
| next->release(); |
There was a problem hiding this comment.
Does this mean before the change, next never got freed? Would this PR fix bitshares/bitshares-core#1006?
There was a problem hiding this comment.
Does this mean before the change,
nextnever got freed?
Yes, in the case if thread::quit() was called the task "async thread::quit()" will be newer freed. I've added "fc::shared_ptr<task_base> next_ptr(next)" in this place just for so as not to explicitly call the "next->release();".
In the case "thread::quit()" an exception is raised from "my->start_next_fiber(true)" and everything will not called that is after (see run_next_task() {... next->run(); ... don't get here }.
In the ~thread_d I check if ready_context has a task and free the last.
There was a problem hiding this comment.
Right now I have no answer. Necessary make additional research.
There was a problem hiding this comment.
Does this mean before the change, next never got freed?
The call to next->release() will delete *next if this was the last reference to it. (retainable and its retain/release methods implement reference counting, used by task_base and fc::shared_ptr. It's a mess, don't ask. I'm working on eliminating this at least partially in bitshares/bitshares-core#1584 .)
There was a problem hiding this comment.
I don't think next->run() will ever throw, so why change this here?
There was a problem hiding this comment.
It's a mess, don't ask.
Yes, you are right absolutely. )
There was a problem hiding this comment.
I don't think
next->run()will ever throw, so why change this here?
Yes, this change is not necessary, as I wrote before, just for so as not to explicitly call release.
|
The memory leak is probably negligible in practice, because we don't create/destroy threads during normal operation. Only on startup/shutdown. Of course it should still be fixed. Thanks for your investigation! |
In thread::quit()
{
...
my->start_next_fiber(true)
...
}
my->start_next_fiber(true) - an exception is raised from here and a context has yet a pointer to the task that will never released and memory will not freed.
So to avoid such situation in ~thread_d() lets release it.
to reproduce run: --tool=memcheck --leak-check=full ./all_tests -t thread_tests