matil019 / haskell-libfuse3

A Haskell binding for libfuse-3.x
https://hackage.haskell.org/package/libfuse3
MIT License
5 stars 1 forks source link

Should we provide blank fuseRelease & fuseReleasedir when fuseOpen or fuseOpendir exists in FuseOperations to prevent memory leak #25

Open TheKK opened 1 year ago

TheKK commented 1 year ago

We got memory leak when newFH is not paired with delFH.

https://github.com/matil019/haskell-libfuse3/blob/f9e0e110a13a9ff9967b492923f30fee3ee511be/src/System/LibFuse3/Internal.hsc#L595 https://github.com/matil019/haskell-libfuse3/blob/f9e0e110a13a9ff9967b492923f30fee3ee511be/src/System/LibFuse3/Internal.hsc#L632

So would that be a good idea to provide corresponding blank cleanup fuse operations (functions that calls delFH) when certain fuse operation exists (functions that calls newFH) but their cleanup operation does not?

Or at least we should document this behavior, since I believe that it's quite hard to figure out this kind of memory leak without reading the source code since RTS profiler doesn't aware the StablePtr layer, and all you got is usage of memory climbs up for unknown reason.

It would be great if you got better idea on this and would like to hear about that :)

matil019 commented 1 year ago

Nice to meet you!

What practical situation are you concerned with? Handles (fh/dhs) are meant to be released with fuseRelease/fuseReleasedir respectively. I think it's the same in the original C libfuse. Also, handles typically carry other resources that requires releasing such as real file handles the filesystem is wrapping, so it's not only the StablePtr but also such resources that leaks when you forget to release it.

The point of the current design is that users would call delFH without knowing it as long as they want to release resources they acquired.

Perhaps you are writing fuseOpen that acquires no resources needing releasing (such as a no-op handler)?


So would that be a good idea to provide corresponding blank cleanup fuse operations (functions that calls delFH) when certain fuse operation exists (functions that calls newFH) but their cleanup operation does not?

The problem is that releasing handles is not trivial. As I explained above, handles typically carry custom resources besides the StablePtr this package allocates. If my understanding is correct, auto-setting cleanup becomes a pitfall in this scenario:

  1. A user adds a fuseOpen that acquires no resources. A trivial fuseRelease, which does delFH only, is silently added. So far so good.
  2. The user decides to add acquiring a resource to fuseOpen but without knowing the need to add fuseRelease.
  3. The trivial fuseRelease calls delFH but doesn't release the custom resource, leaking the resource.

Strictly speaking, the current design has the same pitfall, but arguably better because it encourages writing fuseRelease whenever a user writes fuseOpen, hence reducing the chance that they forget to update it.

Or at least we should document this behavior, since I believe that it's quite hard to figure out this kind of memory leak without reading the source code since RTS profiler doesn't aware the StablePtr layer, and all you got is usage of memory climbs up for unknown reason.

I'm leaning towards to this suggestion. The need to implement fuseRelease/fuseReleasedir when corresponding "open" handlers exist should be emphasized.

TheKK commented 1 year ago

I agree with you, giving a default fuseRelease/fuseReleaseDir isn't that practical in all cases.

My use case is too simple that the fuseOpen/fuseOpenDir only store variables which come from traversing directories on filesystem and no other resource is involved.

Beside adding documentation, what's your opinion about adding a debug message about unpaired fuseOpen/fuseRelease fuseOpenDir/fuseReleaseDir at runtime?