search

postmodern / ffi-hunspell

Ruby FFI bindings for Hunspell.
MIT License
48 stars 24 forks source link

Prevent Memory Leak #9

Closed ebenoist closed 3 years ago

ebenoist commented 10 years ago

There is a memory leak, which I am sure you aware of, when a new Dictionary object is created without being closed. I have some spike code that uses a finalizer to prevent this easy mistake.

I would want to clean it up, test it, and do a performance regression before putting up a PR, but wanted to see if you'd be interested in making this kind of enhancement first.

the83 commented 10 years ago

+1, this bit me the other day.

postmodern commented 10 years ago

Pull Requests welcome. I'm not sure what the safest way to define a finalizer on an ivar within a plain class.

ebenoist commented 10 years ago

@postmodern I have some spike code together, I think the only "gotcha" is if you define the finalizer proc within the instance of the class (then the instance itself is never freed). Moving this to a singleton function should alleviate that issue. I'll get something a little cleaner and tested together in the next few days.

postmodern commented 10 years ago

I think I've seen people put the proc into a class var or constant?

ebenoist commented 10 years ago

I think that would work as well.

postmodern commented 10 years ago

@ebenoist so what I want to know is, how do you (or Ruby) prevent the finalizer from kicking off when you explicitly close the dictionary and destroy the pointer?

ebenoist commented 10 years ago

@postmodern I've been struggling with the same problem actually. I'm not sure yet.

postmodern commented 3 years ago

I may have fixed another memory leak in Dictionary#stem and Dictionary#suggest where Hunspell_free_list wasn't being called on the char ** returned by Hunspell_stem and Hunspell_suggest. ab0c6a36f7d5078f138bb5c4a211920ab6e45b2a

As for this issue with closing the dictionary pointer, maybe we could use FFI::AutoPointer?

postmodern commented 3 years ago

See the auto_pointer branch and 530c548a12d7c4bed300faed18275523e3b6f121.

postmodern commented 3 years ago

Merged into master and released 0.6.1. If there's still memory leaks detected, re-open this issue with fresh analysis.