551824 - Implement a "registry" module that can register and unregister objects

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect)

Description

[Drew Willcoxon :adw]

We're finding a common pattern among our implementations of registering objects to keep track of them and then unregistering them when they're no longer needed.  Often this pattern is used to keep track of things that need to be cleaned up when a module is unloaded.

See also bug 547417 comment 9.

Comment 1

[Atul Varma [:atul]]

Since the 'registry' concept is mainly used for unloading, there's a slightly different pattern I use for this sort of thing...

Basically, the key thing to note is that every object that needs to be "unloaded" actually has an "unload" sort of method; for streams, it's close().  Thus we could imagine adding a function to the unload module called "ensure" that works like this:

  unload.ensure(stream, "close");

The above code is meant to read as "ensure that stream.close() is called on unload, if it hasn't been called yet".  It automatically decorates stream.close() to first check to make sure it hasn't been called yet, automatically queues the method to be called on unload if it hasn't yet been called, and takes care of potential memory leaks and so forth.

An implementation for an API similar (but not identical) to this is here:

  http://hg.mozilla.org/users/avarma_mozilla.com/atul-packages/file/4d716b711232/packages/misc/lib/unload-2.js

Comment 2

[Drew Willcoxon :adw]

That's clever!  At first glance I like it, but it changes the semantics of an object's destroy method.  Part of the stream API is that close()es subsequent to the first throw an exception.  If we used the impl you linked to and ensure() were called on a stream, subsequent closes would be a no-op.  I'm not sure whether one is better than the other, but if there were some class for which subsequent destroys really had some particular meaning, then this pattern wouldn't allow for it.  That's probably something we should discourage, though, hmm.

We'd have to standardize the meaning of multiple destroys across the entire platform, which actually might be a good thing.  Either they always throw an exception -- it would have to be something non-specific like "multiple destroys" -- or they are always no-ops.  What do you think?

Comment 3

[Atul Varma [:atul]]

Good point, yo.  In my implementation of this unload.ensure() function which I linked to in comment 1, the method to "unload the resources of an object" is always called unload() and always has the "ignore subsequent calls to unload() rather than throwing an exception" behavior; but if the close() method is just a synonym for unload() then yeah, weird things happen, b/c it seems like convention that telling a stream to close twice *should* raise an exception.  One alternative may be to actually give the stream object a close() *and* unload() method but simply not document unload(), and have close() just call unload(). (In other words, unload() is basically a private/protected method.)

I'm starting to confuse myself here. Will need to think about this a bit and get back to you, but feel free to offer a solution if you think of one!

Comment 4

[Drew Willcoxon :adw]

> unload(). (In other words, unload() is basically a private/protected method.)

Hmm, but it would be an honor-system private method, no?  Which is to say, not really private.

> I'm starting to confuse myself here. Will need to think about this a bit and
> get back to you, but feel free to offer a solution if you think of one!

That's cool.  The only other thoughts I had were to just make a more generic dictionary or registry module and let each module decide its own fate, or extend your code so that callers of ensure() can define their behavior on multiple destroys, like one of the following:

unload.ensure(stream, "close", function () throw new Error("multiple closes"));
unload.ensure(stream, "close", new Error("multiple closes"));
unload.ensure(stream, "close", "multiple closes");

And for the modules that want no-ops, it's as before:

unload.ensure(obj, "destroy");

It doesn't seem like this bug blocks the text stream or simple storage stuff, right?  We can always improve this later.

Comment 5

[Atul Varma [:atul]]

Yeah, it would be honor-system private. Hmm. There might be a way to make it actually private, though, will have to think about it.

I think that adding an optional parameter to unload.ensure() would be fine, though we might want to consider using keyword parameters at that point (i.e., an options object).

And yeah, I don't think this blocks text stream or simple storage stuff--in fact, there is already committed code in other modules that could take advantage of a registry/unload.ensure-esque mechanism, I think.

Comment 6

[Atul Varma [:atul]]

Note that unload.ensure() is being added to jetpack-core in bug 556940.

Comment 7

[Drew Willcoxon :adw]

Since unload.ensure() was added in bug 556940, marking this wontfix.  Feel free to reopen if you disagree.

Comment 8

[Atul Varma [:atul]]

So I guess we should make a new bug if we want to allow for a second and third optional arg to unload.ensure(), as per your proposal in comment 4?

Comment 9

[Drew Willcoxon :adw]

If you want, please do.  I think my idea might be overkill though. :)  Or maybe I'll run into another use case.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".