search

fukamachi / cl-dbi

Database independent interface for Common Lisp
202 stars 28 forks source link

- memory leak regression #45

Closed cage2 closed 5 years ago

cage2 commented 5 years ago

Hello!

unfortunately i think the commit ea97e62327283bcf41d221f34e4537552fad28be

did not resolve the problem in:

https://github.com/fukamachi/cl-dbi/pull/43

to me seems that the added finalizer is not called and therefore the memory leak is here again.

the commit fcb4d2e3d0139fc8b6b9184ad9194d689058e949 seems OK to me, though.

Thank you for your work! C.

fukamachi commented 5 years ago

I think they'll be finalized when GC collects, but it won't? sqlite library seems to cache 16 queries by default, so small amount of queries may do not work.

fukamachi commented 5 years ago

Since CL-DBI prepared query is intended to be used repeatedly, finalizing them isn't great anyway :(

cage2 commented 5 years ago

On Fri, Feb 01, 2019 at 12:16:36AM -0800, Eitaro Fukamachi wrote:

Hello!

Thank you for you reply!

Since CL-DBI prepared query is intended to be used repeatedly, finalizing them isn't great anyway :(

Well i think i can see the point here, i think it is a good point, indeed! :)

I have somehow managed to make the finalizer called but i got some other scary errors from the cl-sqlite library.

I have no solution unfortunately except an humble suggestion adding a 'free-query' function to release the memory allocated by a prepared query. This function could be added to the public API of this library.

AFAIK only for the sqlite3 driver this function actually would do something useful, but i think in the future some other database engine could benefit from this interface.

Thank you again! C.

cage2 commented 5 years ago

On Fri, Feb 01, 2019 at 12:13:15AM -0800, Eitaro Fukamachi wrote:

I think they'll be finalized when GC collects, but it won't?

It does not, sadly. :(

Moreover the finalizer seems adding some more non deallocated memory.

Anyway FWIW:

 (finalize query
              (lambda ()
                (when (slot-boundp conn-handle 'sqlite::handle)
                  (finalize-statement (query-prepared query)))))))

should became something like (some changing probably needed):

(let ((prepared (query-prepared query)))
  (finalize query
            (lambda ()
              (when (slot-boundp conn-handle 'sqlite::handle)
                (finalize-statement prepared)))))

But the (for me at least)OA, cl-sqlite begin spitting some terrible error messages. :(

Bye! C.

cage2 commented 5 years ago

Hello!

i have written down some code about this idea of an explicit finalizer (for lacks of better word).

https://github.com/cage2/cl-dbi/tree/free-query-resource-poc

Maybe this could became an actual pull request if you agree.

Anyway, I hope this helps! :) Bye! C.

fukamachi commented 5 years ago

That looks good. As depending on GC seems to be troublesome, it could be implemented also for other drivers in the future.

cage2 commented 5 years ago

On Thu, Apr 04, 2019 at 03:02:24AM -0700, Eitaro Fukamachi wrote:

Hello!

That looks good. As depending on GC seems to be troublesome, it could be implemented also for other drivers in the future.

Thank you for reviewing this! :) I'll submit a pull request as soon as possible.

Bye! C.

fukamachi commented 5 years ago

Closing because PR #47 is already merged.