Closed Bug 1025464 Opened 10 years ago Closed 10 years ago

Refactor log sweeping code to use Task.jsm and move it to logger.js

Categories

(Instantbird Graveyard :: Other, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nhnt11, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

The log sweeping code in the stats service can be reused in the near future for indexing, so it should be moved into the logs service and an API should be exposed. Also, the current sweeping code uses multiple recursive functions and can be greatly simplified using Task.jsm.
Attached patch Patch (obsolete) — Splinter Review
This refactors the log sweeping code to use Tasks, and moves it to logger.js. An API is exposed through the imILogger interface, and a callback interface is added (better name required?).

I've tested this on a new profile and a clone of my existing profile, and it seems to work as expected.
Attachment #8440261 - Flags: review?(aleth)
Comment on attachment 8440261 [details] [diff] [review]
Patch

Review of attachment 8440261 [details] [diff] [review]:
-----------------------------------------------------------------

Yay, tasks.

::: chat/components/public/imILogger.idl
@@ +87,5 @@
> +
> +  // Asynchronously iterates through log folders for all prpls and accounts
> +  // and invokes the callback on every log file. Returns a promise that
> +  // resolves when all logs have been parsed.
> +  jsval parseAllLogs(in imIParseLogsCallback aCallback);

I wonder if we should call this method forEach instead.

::: chat/components/src/logger.js
@@ +751,5 @@
> +        }
> +      }
> +      entries = entries.filter(aEntry => aEntry.isDir);
> +      entries.forEach((aEntry, i) => entries[i] = aEntry.path);
> +      return entries;

This is hard to read, how about
return [for (entry of entries) if (entry.isDir) entry.path];

@@ +755,5 @@
> +      return entries;
> +    });
> +
> +    let logsPath = OS.Path.join(OS.Constants.Path.profileDir, "logs");
> +    let dirFilter = aEntry => aEntry.isDir;

unused.

@@ +764,5 @@
> +      yield getAllSubdirs(accounts, "Error while sweeping account folder:");
> +    for (let folder of logFolders) {
> +      let iterator = new OS.File.DirectoryIterator(folder);
> +      try {
> +        yield iterator.forEach(aEntry => {

Shouldn't this callback return a promise?
Attachment #8440261 - Flags: review?(aleth) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8440808 - Attachment description: movelogsweeper.patch → Patch v2
Attachment #8440808 - Flags: review?(aleth)
Attached patch Patch v2 (obsolete) — Splinter Review
The previous patch got submitted somehow before I could edit any details, and I can't seem to interdiff it - so I'm attaching it again. Sorry for the inconvenience.
Attachment #8440808 - Attachment is obsolete: true
Attachment #8440808 - Flags: review?(aleth)
Attachment #8440810 - Flags: review?(aleth)
Comment on attachment 8440810 [details] [diff] [review]
Patch v2

Review of attachment 8440810 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/components/src/logger.js
@@ +731,5 @@
>      // If there was an error, this will return an EmptyEnumerator.
>      return this._getEnumerator(entries, aGroupByDay);
>    }),
>  
> +  parseAllLogs: Task.async(function* (aCallback) {

forEach.

This makes me think this patch has not been tested ;)

How about adding a simple test? Shouldn't take more than a couple of minutes as you already populate a profile with logs.

@@ +763,5 @@
> +        yield iterator.forEach(Task.async(function* (aEntry) {
> +          if (aEntry.isDir || !aEntry.name.endsWith(".json"))
> +            return;
> +          try {
> +            yield aCallback.parseLog(aEntry.path);

I think it might be better to simply
return aCallback.parseLog(aEntry.path)
and not have a try/catch here. Saves us the Task.async, and allows the callback to decide whether an error is serious enough to warrant cancelling the parsing of the other logs in the same folder.
Attachment #8440810 - Flags: review?(aleth) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
Address review comments.
Attachment #8440261 - Attachment is obsolete: true
Attachment #8440810 - Attachment is obsolete: true
Attachment #8440879 - Flags: review?(aleth)
Attached patch Patch v4 (obsolete) — Splinter Review
Improved error handling, updated a comment.
Attachment #8440879 - Attachment is obsolete: true
Attachment #8440879 - Flags: review?(aleth)
Attachment #8441059 - Flags: review?(aleth)
Attached patch Patch v4.01 (obsolete) — Splinter Review
Well, this is embarrassing.
Fixed another parseAllLogs -> forEach that I'd missed.
Attachment #8441059 - Attachment is obsolete: true
Attachment #8441059 - Flags: review?(aleth)
Attachment #8441133 - Flags: review?(aleth)
Comment on attachment 8441133 [details] [diff] [review]
Patch v4.01

Review of attachment 8441133 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/components/src/logger.js
@@ +761,5 @@
> +      let iterator = new OS.File.DirectoryIterator(folder);
> +      try {
> +        yield iterator.forEach(aEntry => {
> +          if (aEntry.isDir || !aEntry.name.endsWith(".json"))
> +            return;

Maybe this should return true or something to avoid "function does not always return a value" warnings.

@@ +767,5 @@
> +        });
> +      } catch (aError) {
> +        // If the callback threw, reject the promise and let the caller handle it.
> +        if (!(aError instanceof OS.File.Error))
> +          throw aError;

Does this throw prevent the iterator.close from happening?
Attached patch Patch v5 (obsolete) — Splinter Review
> ::: chat/components/src/logger.js
> @@ +761,5 @@
> > +      let iterator = new OS.File.DirectoryIterator(folder);
> > +      try {
> > +        yield iterator.forEach(aEntry => {
> > +          if (aEntry.isDir || !aEntry.name.endsWith(".json"))
> > +            return;
> 
> Maybe this should return true or something to avoid "function does not
> always return a value" warnings.

I've made it return null as we discussed.

> @@ +767,5 @@
> > +        });
> > +      } catch (aError) {
> > +        // If the callback threw, reject the promise and let the caller handle it.
> > +        if (!(aError instanceof OS.File.Error))
> > +          throw aError;
> 
> Does this throw prevent the iterator.close from happening?

Nope. Code in a finally block is called even if the catch block throws. (tested this personally)
Attachment #8441133 - Attachment is obsolete: true
Attachment #8441133 - Flags: review?(aleth)
Attachment #8441338 - Flags: review?(aleth)
Comment on attachment 8441338 [details] [diff] [review]
Patch v5

Review of attachment 8441338 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8441338 - Flags: review?(aleth) → review+
Comment on attachment 8441338 [details] [diff] [review]
Patch v5

Review of attachment 8441338 [details] [diff] [review]:
-----------------------------------------------------------------

Note: I haven't looked at the implementation, but I'm not too worried about it if aleth r+'ed it :-).

::: chat/components/public/imILogger.idl
@@ +51,5 @@
>    jsval getConversation();
>  };
>  
> +[scriptable, function, uuid(2ab5f8ac-4b89-4954-9a4a-7c167f1e3b0d)]
> +interface imIParseLogsCallback: nsISupports {

Please remove the word 'parse' from this interface name, as this seems a generic way to iterate over the logs, and the callback could do plenty of other things (eg. compute a hash, make a backup of the file somewhere, ...).

Possible words to use instead: handle, process

@@ +53,5 @@
>  
> +[scriptable, function, uuid(2ab5f8ac-4b89-4954-9a4a-7c167f1e3b0d)]
> +interface imIParseLogsCallback: nsISupports {
> +  // The callback can return a promise. If it does, then it will not be called
> +  // on the next log until this promise resolves.

What happens if the promise rejects? (I see you've documented it on the forEach method though :-))

@@ +54,5 @@
> +[scriptable, function, uuid(2ab5f8ac-4b89-4954-9a4a-7c167f1e3b0d)]
> +interface imIParseLogsCallback: nsISupports {
> +  // The callback can return a promise. If it does, then it will not be called
> +  // on the next log until this promise resolves.
> +  jsval parseLog(in AUTF8String aLogPath);

Same here.
Attachment #8441338 - Flags: review-
Comment on attachment 8441338 [details] [diff] [review]
Patch v5

Review of attachment 8441338 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/components/src/logger.js
@@ +747,5 @@
> +        }
> +      }
> +      entries = entries.filter(aEntry => aEntry.isDir);
> +      entries = entries.map(aEntry => aEntry.path);
> +      return entries;

do you like the shorter:
return entries.filter(...)
              .map(...);
?

::: im/components/ibConvStatsService.js
@@ +118,5 @@
> +      statsService._cacheAllStats(); // Flush stats to JSON cache.
> +      statsService._convs.sort(statsService._sortComparator);
> +      statsService._notifyObservers("log-sweeping", "done");
> +      gStatsByContactId = {}; // Initialize stats cache for contacts.
> +    });

Do you need to handle promise rejections here?
Attachment #8441338 - Flags: feedback+
Attached patch Patch v6Splinter Review
Address review/feedback comments.
Thanks for the tip about handling rejections, I didn't think I needed to (forEach already does Cu.reportError where necessary) but you made me look at it again and I realized I needed to set the error flag.
Attachment #8441338 - Attachment is obsolete: true
Attachment #8446872 - Flags: review?(florian)
Attachment #8446872 - Flags: review?(florian) → review+
https://hg.mozilla.org/comm-central/rev/de14c1c218bc
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.