Developer forums (C::B DEVELOPMENT STRICTLY!) > Development
HUGE MEMORY LEAK pinpointed!
rickg22:
--- Quote from: dmoore on June 24, 2007, 03:15:36 am ---[
in my patch, I NULLed out the loader member of opts in all instances where Parse is called and needs a loader, so (!opts.loader) should always be true. More importantly, in the present state of the plugin, the loader should never be called (using Load) outside of Parse or there will be memory leaks, so loader must always be NULL unless you come up with a more elaborate patch.
--- End quote ---
Yes, but since we're dealing with critical code here, we can't leave anything to luck or past decisions. The code might be changed later, and this line could be forgotten. So, please put the if(!opts.loader) there. It's safe and it doesn't harm.
dmoore:
ok, and i've added a couple of lines to produce a debug warning about memory leaks if the loader has already been set.
thomas:
--- Quote from: rickg22 on June 24, 2007, 02:00:50 am ---Thomas, what do you think about the patch? Is it OK with you?
--- End quote ---
I don't understand what either the original code or the patch is doing :)
In particular, I don't understand what Parse(...., NULL) actually parses, but if it works, that's ok with me :)
--- Quote from: dmoore on June 24, 2007, 03:15:36 am ---deletes the loaders that aren't complete (this causes seg faults)
--- End quote ---
Yes, necessarily. The loader object that you get back is aliased in the file manager's queue, so its worker thread knows what to load. If you delete the loader before it has finished loading, FileManager will read from and write to unallocated memory.
This behaviour was accepted when the class was created, as it keeps the class a lot less complicated and it shouldn't be an issue, normally. You don't usually load a dozen files if you don't really want to do anything with them. However, it seems like this assumption may have been wrong.
--- Quote ---There are lots of different ways to address this issue - my approach was just the simplest that came to mind.
--- End quote ---
Probably the best solution will be to take away object ownership from the user. It just doesn't work...
Der Meister:
--- Quote from: thomas on June 24, 2007, 04:06:25 pm ---
--- Quote ---There are lots of different ways to address this issue - my approach was just the simplest that came to mind.
--- End quote ---
Probably the best solution will be to take away object ownership from the user. It just doesn't work...
--- End quote ---
I agree with that. But that means that the FileLoader is resonsible for freeing the memory. But therefore it has to know whether it is still in use or not required any more. But I see only one way to implement this: smart pointers... Well...
dmoore:
--- Quote from: Der Meister on June 24, 2007, 04:14:21 pm ---
--- Quote from: thomas on June 24, 2007, 04:06:25 pm ---
--- Quote ---There are lots of different ways to address this issue - my approach was just the simplest that came to mind.
--- End quote ---
Probably the best solution will be to take away object ownership from the user. It just doesn't work...
--- End quote ---
I agree with that. But that means that the FileLoader is resonsible for freeing the memory. But therefore it has to know whether it is still in use or not required any more. But I see only one way to implement this: smart pointers... Well...
--- End quote ---
is it not possible to use events? when file loading is completed the loader could send a message to the caller saying "here's the data, let me know when you are done (by calling certain member of the loader) so i can clean up"
Navigation
[0] Message Index
[#] Next page
[*] Previous page
Go to full version