Closed Bug 955707 Opened 10 years ago Closed 10 years ago

Log sweeping code should close its iterators and handle errors better

Categories

(Instantbird Graveyard :: Other, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

(Whiteboard: [1.5-blocking])

Attachments

(1 file, 1 obsolete file)

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

Split off from bug 955653 (bio 2208).
Depends on: 955653
Attached patch Patch (obsolete) — Splinter Review
*** 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.
Attachment #8354869 - Flags: review?(florian)
Whiteboard: [1.5-blocking]
*** 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();?
Attached patch Patch2Splinter Review
*** 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).
Attachment #8354880 - Flags: review?(florian)
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 ***
Attachment #8354869 - Attachment is obsolete: true
Attachment #8354869 - Flags: review?(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 ***
Attachment #8354880 - Flags: review?(florian) → review+
Whiteboard: [1.5-blocking] → [1.5-blocking][checkin-needed]
*** Original post on bio 2258 at 2013-12-01 23:31:55 UTC ***

http://hg.instantbird.org/instantbird/rev/7882528028ea
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [1.5-blocking][checkin-needed] → [1.5-blocking]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.