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)
Instantbird Graveyard
Other
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: aleth, Assigned: aleth)
References
Details
(Whiteboard: [1.5-blocking])
Attachments
(1 file, 1 obsolete file)
9.30 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 2258 at 2013-11-27 19:02:00 UTC *** Split off from bug 955653 (bio 2208).
Assignee | ||
Comment 1•10 years ago
|
||
*** 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)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [1.5-blocking]
Comment 2•10 years ago
|
||
*** 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();?
Assignee | ||
Comment 3•10 years ago
|
||
*** 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)
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Whiteboard: [1.5-blocking] → [1.5-blocking][checkin-needed]
Comment 6•10 years ago
|
||
*** 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.
Description
•