547417 - Implement text streams

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

Attachment: patch 1

While simple storage could be implemented on top of the binary streams in bug 541622, it's better to use text streams since JSON is textual data.  Cuddlefish will need them anyway, right?

* Changes from byte streams patch:

Dropped the |begin| and |end| params from stream.write().  They're specified in one of the CommonJS stream proposals, but I think they're unnecessary when all you're doing is writing a string.  It's easy for consumers to make a substring themselves.  It simplifies the API and means less code and tests for us.

Added async streams to read/write on a background thread.  Simple storage will use the async writer at least.  To open a file stream in async mode, put a "@" in open()'s mode string...

Split out my tests into a new file.

Added an unloader test per the best practices guide.

xpcom.throwFriendlyError() -> throw xpcom.friendlyError().

* Questions:

The Reader/Writer constructors take a Gecko stream.  Given that Cuddlefish is supposed to wrap Gecko, what's our policy on Gecko objects at API entry/exit points?

The async streams take callbacks.  Would it be better to do a more general stream observer pattern?

Comment 1

[Drew Willcoxon :adw]

There are synchronization issues with the async streams.  I'm not so much worried about concurrent read/writes as I am about a concurrent read/write and close, since the module can be unloaded at any time.  There're no synchronization primitives exposed to JS from XPCOM AFAIK.

Potential solutions?

1. Async streams aren't closed when the module is unloaded.  Trust that consumers will close them when they're done.  If they try to read/write concurrently or read/write and close concurrently, they'll get errors.

2. Add static methods on the module that open the stream, read/write the whole thing, then close -- all in one shot.

What do you think?

Comment 2

[Atul Varma [:atul]]

Hmm, it could be helpful to investigate how Firefox deals with this at the platform level and how OS's deal with this at the process level. My understanding is that Firefox actually sends a sort of "quit request notification" out to all observers before it actually starts shutting down, at which point anything doing something vital has the opportunity to tell Firefox to wait a moment before shutting down (or web pages have the ability to bring up dialog boxes to the user).  We could potentially introduce a similar mechanism to Jetpack, essentially allowing for some async actions to be carried out no matter what.

I'd prefer not to pursue option #1; ideally all these low-level modules like file will make things so that everything gets cleaned-up when a module is unloaded, so that jetpack code doesn't have to deal with any kind of resource management outside of the context of their jetpack's lifecycle.

More thoughts later...

Comment 3

[Drew Willcoxon :adw]

(In reply to comment #2)
> Hmm, it could be helpful to investigate how Firefox deals with this at the
> platform level and how OS's deal with this at the process level. My
> understanding is that Firefox actually sends a sort of "quit request
> notification" out to all observers before it actually starts shutting down, at
> which point anything doing something vital has the opportunity to tell Firefox
> to wait a moment before shutting down (or web pages have the ability to bring
> up dialog boxes to the user).  We could potentially introduce a similar
> mechanism to Jetpack, essentially allowing for some async actions to be carried
> out no matter what.

AFAIK all threads join the main thread before app shutdown.  For instance, Storage spins a background thread for its async calls, but it doesn't have to specially handle app shutdown.  It just calls Shutdown() on the thread when the module is destroyed (which is something this patch needs to do).

I'm more concerned about the basic synchronization problem.  If this were C++ I'd use a lock, it wouldn't be a problem.

> I'd prefer not to pursue option #1; ideally all these low-level modules like
> file will make things so that everything gets cleaned-up when a module is
> unloaded, so that jetpack code doesn't have to deal with any kind of resource
> management outside of the context of their jetpack's lifecycle.

Yeah, I agree.  I think option 2 is what I'll do unless you had more thoughts.

Comment 4

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

Marking P1, as this is a prerequisite for simple storage, which is P1 for 0.2.

Comment 5

[Drew Willcoxon :adw]

Attachment: patch 2

This is based on the apparently new jetpack-core lib rather than cuddlefish and cleans up some byte stream stuff necessary for doing text streams.  I'll file another bug to replace the byte stream tests, add support for opening files in binary mode, and other byte stream cleanup.

Async streams can still be opened, but they're closed immediately after the first read/write or on module unload, whichever comes first.  Attempts to concurrently read/write or read/write and close throw errors.

The Reader/Writer constructors take a Gecko stream.  Given that Cuddlefish or Jetpack or whatever is supposed to wrap Gecko, what's the policy on Gecko objects at API entry/exit points?

Comment 6

[Drew Willcoxon :adw]

Attachment: patch 2.1

Small change, ensures that file.read and the async streams' read/write close the stream in the presence of errors.

Comment 7

[Drew Willcoxon :adw]

Attachment: patch 2.2

More bugs, wasn't calling xpcom.friendlyError properly.

Comment 8

[Atul Varma [:atul]]

Sorry it took so long for me to get back to you on this.

Regarding your question in comment 5, well, that's a good question.  At the very least, I think that we should document arguments and return values involving "raw" XPCOM objects as such, perhaps even marking methods that take or return such objects as "unsafe" because they can't necessarily be securitized, function pointers passed into them don't necessarily get freed when parent modules are unloaded, and so forth. There's actually already some modules in jetpack-core like this, e.g. the observer-service module that does a fine job of managing observers, but provides raw/unsafe XPCOM 'subject' arguments when notifying them.

We'll have to add/change the glossary [1] to reflect this notion as well.

[1] https://jetpack.mozillalabs.com/sdk/0.1/docs/#guide/glossary

Comment 9

[Atul Varma [:atul]]

I've been looking over your code and I'm wondering if you think it'd be a good idea to split this patch up into a few separate pieces, each of which has its own bug and is reviewed separately, just to make things more digestible.

It seems like this could be split up into the following:

* an 'xpcom.friendlyError' patch that changes xpcom.js, byte-streams.js, and any other existing code that used xpcom.throwFriendlyError().

* a 'worker-thread.js' patch introducing a new worker-thread module with a Worker class in it (i.e., factoring-out the Worker class from text-streams.js in this patch).

* Either you or I should make a patch that effectively adds the 'registry' class to unload.js. I made something in my personal repo that does the same kind of thing, but as I created mine for a slightly different use case, we may want to figure out what kinds of properties we want the registry mechanism to have.

* a 'text-streams.js' patch that adds the text-streams module and changes file.js to use it (I suppose the latter could be its own separate patch, but it'd be so small that I'm not sure how much it'd accomplish to separate them).

Does any of that sound reasonable? Part of the reason I think this might be useful is b/c it'd effectively create separate 'threads' for each area that's being patched, rather than one mega-thread where discussion of all these distinct issues is smooshed together, potentially leading to confusion or forgetfulness.

Comment 10

[Atul Varma [:atul]]

I just added a new bug reflecting what needs to be done about comment 5 and comment 8. It is bug 551815.

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 428903 [details] [diff] [review]
patch 2.2

Yeah, comment 9 is very reasonable.

> * an 'xpcom.friendlyError' patch that changes xpcom.js, byte-streams.js, and
> any other existing code that used xpcom.throwFriendlyError().

I can file this.

> * a 'worker-thread.js' patch introducing a new worker-thread module with a
> Worker class in it (i.e., factoring-out the Worker class from text-streams.js
> in this patch).

This too.

> * Either you or I should make a patch that effectively adds the 'registry'
> class to unload.js. I made something in my personal repo that does the same
> kind of thing, but as I created mine for a slightly different use case, we may
> want to figure out what kinds of properties we want the registry mechanism to
> have.

I'll file and we can discuss it there.

> * a 'text-streams.js' patch that adds the text-streams module and changes
> file.js to use it (I suppose the latter could be its own separate patch, but
> it'd be so small that I'm not sure how much it'd accomplish to separate them).

Let's morph this bug into that work.

Thanks Atul!

Comment 12

[Drew Willcoxon :adw]

(In reply to comment #9)
> * an 'xpcom.friendlyError' patch that changes xpcom.js, byte-streams.js, and
> any other existing code that used xpcom.throwFriendlyError().

bug 551821

> * a 'worker-thread.js' patch introducing a new worker-thread module with a
> Worker class in it (i.e., factoring-out the Worker class from text-streams.js
> in this patch).

bug 551822
 
> * Either you or I should make a patch that effectively adds the 'registry'
> class to unload.js. I made something in my personal repo that does the same
> kind of thing, but as I created mine for a slightly different use case, we may
> want to figure out what kinds of properties we want the registry mechanism to
> have.

bug 551824

Comment 13

[Atul Varma [:atul]]

Awesome, thanks Drew!

Comment 14

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

(In reply to comment #4)
> Marking P1, as this is a prerequisite for simple storage, which is P1 for 0.2.

Simple Storage was deferred, and we haven't yet scheduled it for another release, so unsetting the target milestone in the meantime.  However, Drew: feel free to update it if you have a sense of when this work will land.

Comment 15

[Drew Willcoxon :adw]

Attachment: patch v3

This is rewritten to hopefully be simpler.  It still lets you write asynchronously, but now it uses the safe NetUtil, which was made for that purpose, rather than spinning up new threads.

This patch makes one test in test-file.js redundant, the one that reads and writes.  That test currently tests byte streams, but with the change to file.open(), it would now be testing text streams, which has its own patch, so I've removed it.  If it's OK I'd like to file a followup bug to replace the byte streams tests.  There are also some other related refactorings of test-file.js.

Comment 16

[Drew Willcoxon :adw]

Attachment: patch v3.1

Spotted a typo in the docs and added a comment about nsIConverterOutputStream that I meant to add earlier.

Comment 17

[Atul Varma [:atul]]

Comment on attachment 443768 [details] [diff] [review]
patch v3.1

This looks good, though it seems like there should be a way to get around having to create and unit test this registry class, especially now that we have unload.ensure() around. Here's one way, which also gets rid of some duplicate code shared between the reader and writer...

Define a private class that's only used by the module:

  function StreamManager(stream) {
    this.stream = stream;
    this.opened = true;
    require("unload").ensure(this);
  }
  
  StreamManager.prototype = {
    ensureOpened: function ensureOpened() {
      if (!this.opened)
        throw closedError();
    },
    unload: function unload() {
      this.stream.close();
      this.opened = false;
    }
  }

Then within the closure of the constructors:

  var manager = new StreamManager(stream);

  this.close = function TextReader_close() {
    manager.ensureOpened();
    manager.unload();
  };

Urg, I'm still not happy with that, but hopefully you get the idea. I guess the StreamManager-type class could also be used by the byte-streams module too... Anyways, just an idea.

Comment 18

[Drew Willcoxon :adw]

Attachment: patch v3.2

Thanks Atul.  Asking for review again just to be safe, mainly for the first change mentioned below.

Adds a StreamManager class as you suggested, but it's extended to also define closed and close() on the stream in order to remove more duplicated code.

Adds checkCharset() to remove yet more duplicated code.

>     unload: function unload() {
>       this.stream.close();
>       this.opened = false;
>     }

Since writeAsync() causes the writer to close, I need to actually check whether opened is true before closing the stream.  Given that, when() feels more natural to me, so I've used it instead.

If duplication of the lifetime-management code weren't an issue, the registry singleton could be replaced with this in the stream constructors:

  require("unload").when(function () {
    if (opened)
      self.close();
  });

Comment 19

[Atul Varma [:atul]]

Comment on attachment 444039 [details] [diff] [review]
patch v3.2

So the only thing that sucks about unload.when() is that, since I just copied it from Narwhal, its semantics are different from ensure. :(

With ensure(), once the unload() method is called the first time--either explicitly by some other code or when unload.send() is called--it automatically gets removed from an internal array in the unload module, which lets it get garbage collected.

However, unload.when() is much simpler but more limited: for one thing, there's not actually any way to 'unregister' a callback registered with it, and for another thing, it never actually "wraps" the callback in any way to ensure that it only gets called once, nor is it ever removed from the internal list of unload functions.

I don't like this difference in semantics--if it tripped you up, it's likely to trip up lots more people, so ideally we should change the unload() module's exported functions or name them to be more clear about what they're doing.

In the meantime, though, you might want to just change your code to use unload.ensure() instead--otherwise that callback you register when each StreamManager is created is never released until the entire extension is unloaded, which in turn means the StreamManager itself leaks, I think, since the callback it registers has the StreamManager's constructor as part of its local scope.

Comment 20

[Drew Willcoxon :adw]

I'm aware of the difference between when() and ensure().  But I didn't consider whether the unload module's implementation maintains a persistent reference to its clients' callbacks.

A few questions:

Why doesn't send() remove observers as it notifies them?  When will send() be called more than once for a given instance of the unload module?

Or, why doesn't when() wrap its callback so that it first removes itself from the list of observers?

Also, byte-streams uses when().  It doesn't call it in a constructor, but it calls it in the module's global scope, so the ByteReader constructor, ByteWriter constructor, and streamRegistry singleton are leaked.  That's better than leaking instances, but they're still leaks.  Is it not possible to use when() without leaking?

Comment 21

[Atul Varma [:atul]]

(In reply to comment #20)
> Why doesn't send() remove observers as it notifies them?  When will send() be
> called more than once for a given instance of the unload module?

Solely because it was part of the module when I brought it over from the Narwhal source code. :(  Because none of our code actually relies on this fact, though, I think we can change it without any ill effects.

> Or, why doesn't when() wrap its callback so that it first removes itself from
> the list of observers?

Ditto...

> Also, byte-streams uses when().  It doesn't call it in a constructor, but it
> calls it in the module's global scope, so the ByteReader constructor,
> ByteWriter constructor, and streamRegistry singleton are leaked.  That's better
> than leaking instances, but they're still leaks.  Is it not possible to use
> when() without leaking?

Actually, they don't really leak because when they're called at the module scope, they're effectively tying their callbacks to the lifetime of their module, which is the same as the lifetime of the unload module. If that makes sense. In fact, the only time that calling unload.when() really makes sense at present, in general, is when they're called at the module scope.

Comment 22

[Drew Willcoxon :adw]

Attachment: patch v4

Comment 23

[Atul Varma [:atul]]

Comment on attachment 444208 [details] [diff] [review]
patch v4

Awesome. Ship it!

Comment 24

[Drew Willcoxon :adw]

http://hg.mozilla.org/labs/jetpack-sdk/rev/675fd6c24317

Comment 25

[Drew Willcoxon :adw]

(In reply to comment #15)
> This patch makes one test in test-file.js redundant, the one that reads and
> writes.  That test currently tests byte streams, but with the change to
> file.open(), it would now be testing text streams, which has its own patch, so
> I've removed it.  If it's OK I'd like to file a followup bug to replace the
> byte streams tests.

bug 565083

Comment 26

[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".