777715 - [OS.File] Finalization for async API

Status: RESOLVED WONTFIX

(Toolkit Graveyard :: OS.File, enhancement)

Description

[David Teller [:Yoric] - still alive but not very active]

With a little hack, we could add finalization to the async API.

Here is the process:
- in osfileutils.{h, cpp}, add the two following functions 
   void osfile_register_leaked_file_handle(handle handle)
   bool osfile_reset_leaked_file_handles(handle** handles, size_t* size)
- use osfile_register_leaked_file_handle as finalizer in osfile_async_front.jsm
- every so often (once per minute, for instance?), call osfile_reset_leaked_file_handles from osfile_async_front.jsm - if the array is not empty, schedule all leaked handles for closing.

Further, we may use this mechanism for bug 777712.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. An implementation of weak finalizers (js code)

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Adapting OS.File to use weak finalizers

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Companion testsuite

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: 4. Native support code

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attaching a prototype implementation (plenty of debug code) of finalization for async OS.File. List of the nice features:
- leaked files are detected/collected one by one;
- whenever a file leaks, we know its name, which should be quite helpful for debugging leaks (in debug mode, we could also capture the file creation stack to display further information);
- runtime penalty should be extremely low when no file leaks.

Comment 6

[(dormant account)]

Comment on attachment 649347 [details] [diff] [review]
2. Adapting OS.File to use weak finalizers

always() sounds awkward. Maybe .cleanup() or .finalize() would be better? Should also upstream this into the promise lib(but I wouldn't block on that).

This looks ok.

Comment 7

[(dormant account)]

Comment on attachment 649345 [details] [diff] [review]
1. An implementation of weak finalizers (js code)

This is overly complicated. Instead of poorly emulating finalizers...why not just store disposable fds in the map in the worker?

Comment 8

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #6)
> Comment on attachment 649347 [details] [diff] [review]
> 2. Adapting OS.File to use weak finalizers
> 
> always() sounds awkward. Maybe .cleanup() or .finalize() would be better?
> Should also upstream this into the promise lib(but I wouldn't block on that).
> 
> This looks ok.

Well, neither "clean up" nor "finalize" sound quite match the semantics. Ideally, this would be "finally", but this is already a keyword.

Also, upstream has rejected this feature several months ago, so...

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #7)
> Comment on attachment 649345 [details] [diff] [review]
> 1. An implementation of weak finalizers (js code)
> 
> This is overly complicated. Instead of poorly emulating finalizers...why not
> just store disposable fds in the map in the worker?

We will need to discuss this on Skype. But to make this short, storing disposable fds in the map in the worker tends to prevent finalization rather than improving it.

Comment 10

[(dormant account)]

(In reply to David Rajchenbach Teller from comment #9)
> 
> We will need to discuss this on Skype. But to make this short, storing
> disposable fds in the map in the worker tends to prevent finalization rather
> than improving it.

Doesn't the map go away when worker goes away, causing gc to run finalizers?

Comment 11

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #10)
> (In reply to David Rajchenbach Teller from comment #9)
> > 
> > We will need to discuss this on Skype. But to make this short, storing
> > disposable fds in the map in the worker tends to prevent finalization rather
> > than improving it.
> 
> Doesn't the map go away when worker goes away, causing gc to run finalizers?

It does. However, I am rather unconvinced that we can find a reasonable API in which the worker effectively goes away.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Let me argue a little further.

The first use scenario for OS.File is the page thumbnails. During the whole duration of the Firefox process, this component writes new files, and every so often it removes a bunch of them. Since the component requires writing new files permanently, there is no clear moment at which we can release the worker. Consequently, if we rely upon garbage-collection of the worker to close leaking files, I see three possible strategies:
1/ we never release the worker => leaking files are never closed;
2/ we release the worker after every single write => we kill the CPU *and* we make the sourcecode of the component more complicated;
3/ we find a weird strategy => we complicate the source code of the component *and* we have no way to test our result *and* we need to invent yet another strategy for the next client.

By opposition, with the patches attached to this bug
- no garbage-collection specific code is required in the client;
- leaking files are closed transparently;
- the developer is informed of leaking files, which should help them debugging;
- plus the testsuite attached to this bug covers file leaks.

Comment 13

[(dormant account)]

I do not like more timers being added, I do not like more complexity either.
My main problem with the previous patch is that it would leak handles even on worker exit.

Lets go with a solution that only cleans up handles on worker exit. It's a minimal safety valve, clients should still be expected to close their files. No need to overdesign a more active handle gc.

I'm also not convinced that thumbnail service can't batch up writes and only run the worker occasionally :)

Comment 14

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #13)
> I do not like more timers being added, I do not like more complexity either.
> My main problem with the previous patch is that it would leak handles even
> on worker exit.

I really believe that we are not talking about the same thing (also, I do not know which one is the "previous patch", there are many of these).

In the current design of OS.File, the one I believe we have agreed upon, there are two families of usage scenarios:
1/ either users want to do some I/O in a worker thread they manage themselves; or
2/ users want to do some async I/O from the main thread, and we ensure that I/O actually happens off-main-thread.

I suspect that you are talking about family 1/. This one has landed a few weeks ago on m-c. The version currently on m-c auto-closes leaking files both when the worker exits and when garbage-collection strikes. As far as I can tell, there is nothing more to do on the front of finalization for this family, unless you want me to make it less fine-grained and only autoclose at worker exit.

On the other hand, for the past few messages, I have been talking about family 2/ which I have been developing for the past few weeks. In this family, a small pool of workers (possibly only one worker), handled by OS.File itself, is shared transparently by all API clients. Most likely, *workers never exit*, so closing files at worker exit is not an option.

> Lets go with a solution that only cleans up handles on worker exit. It's a
> minimal safety valve, clients should still be expected to close their files.
> No need to overdesign a more active handle gc.

In family 2/, can you think of even one scenario in which cleaning handles on worker exit would help?

Comment 15

[(dormant account)]

(In reply to David Rajchenbach Teller from comment #14)

i was walking about fam2

> In family 2/, can you think of even one scenario in which cleaning handles
> on worker exit would help?

testing. we should be able to ensure that no handles are open from js at certain points.

i'm also not sold on a permanent io thread as the only option. Ondemand workers might be handy too.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #15)
> (In reply to David Rajchenbach Teller from comment #14)
> 
> i was walking about fam2
> 
> > In family 2/, can you think of even one scenario in which cleaning handles
> > on worker exit would help?
> 
> testing. we should be able to ensure that no handles are open from js at
> certain points.

I do not understand what you mean.

> i'm also not sold on a permanent io thread as the only option. Ondemand
> workers might be handy too.

Ok, but that sounds very much like family 3, then. I would really like to finish family 2 first. Since, well, basically, it already works.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Closing this bug. If anybody needs the feature, don't hesitate to reopen it.