955707 - Log sweeping code should close its iterators and handle errors better

Status: RESOLVED FIXED

(Instantbird Graveyard :: Other, defect)

Description

[aleth [:aleth]]

*** Original post on bio 2258 at 2013-11-27 19:02:00 UTC ***

Split off from bug 955653 (bio 2208).

Comment 1

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 2258 as attmnt 3087 at 2013-11-27 19:12:00 UTC ***

> >+  // Sweep the log folders.
> >   _sweepLogFolders: function() {
> 
> >     let decoder = new TextDecoder();
> >+    iterator.forEach(aEntry => {
> >+      if (aEntry.name.endsWith(".json"))
> >+        this._sweepJSONLog(aEntry.path, decoder);
> 
> Unless I'm seriously confused, this function schedules reading the JSON log
> file and returns immediately. The iterator's forEach can accept promises as
> return values from the callback and wait for them... but the callback doesn't
> return anything here.

Good catch. The funny thing is it was actually behaving correctly even before I fixed this and now I don't understand why.

> >+    }).then(
> >+      (function() {
> 
> >+      }).bind(this),
> 
> It's strange to have one last function using bind here, when everything else
> has been converted to the => shorthand.

Turns out it's actually possible to use the shorthand even when there are no arguments.

> >   _cacheAllStats: function() {
> 
> >+    // Don't cache anything if we encountered an error during log sweeping, so
> >+    // a fresh log sweep is triggered on next startup.
> >+    if (gLogParser.logSweepingError)
> >+      return;
> 
> I'm not convinced this is the right thing to do. I think we need to make a
> difference between "there was a fatal error" and "there were a few files we
> couldn't read".

The current patch ignores corrupt (but readable) log files. Other errors mean the results of the log sweep are used for the current session, but we will sweep again after a restart.

Comment 2

[Florian Quèze [:florian]]

*** Original post on bio 2258 at 2013-11-27 21:56:01 UTC ***

Comment on attachment 8354869 [details] [diff] [review] (bio-attmnt 3087)
Patch

>+    iterator.forEach(aEntry => {
>+      if (aEntry.name.endsWith(".json"))
>+        return this._sweepJSONLog(aEntry.path, decoder);
>+    }).then(

Won't this cause a "function doesn't always return a value" warning?

In the other case, should the return value be something like new Promise().resolve();?

Comment 3

[aleth [:aleth]]

Attachment: Patch2

*** Original post on bio 2258 as attmnt 3097 at 2013-11-28 16:16:00 UTC ***

>>+    iterator.forEach(aEntry => {
>>+      if (aEntry.name.endsWith(".json"))
>>+        return this._sweepJSONLog(aEntry.path, decoder);
>>+    }).then(
>
>Won't this cause a "function doesn't always return a value" warning?

Yes. I added a return undefined; (no need to create a Promise if we want to continue immediately).

Comment 4

[aleth [:aleth]]

Comment on attachment 8354869 [details] [diff] [review]
Patch

*** Original change on bio 2258 attmnt 3087 at 2013-11-28 16:16:58 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8354880 [details] [diff] [review]
Patch2

*** Original change on bio 2258 attmnt 3097 at 2013-12-01 17:02:24 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[Patrick Cloke [:clokep]]

*** Original post on bio 2258 at 2013-12-01 23:31:55 UTC ***

http://hg.instantbird.org/instantbird/rev/7882528028ea