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

Status: RESOLVED FIXED

(Instantbird Graveyard :: Other, defect)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

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.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch

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.

Comment 2

[aleth [: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?

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2

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.

Comment 5

[aleth [: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.

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v3

Address review comments.

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v4

Improved error handling, updated a comment.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v4.01

Well, this is embarrassing.
Fixed another parseAllLogs -> forEach that I'd missed.

Comment 9

[aleth [: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?

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v5

> ::: 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)

Comment 11

[aleth [:aleth]]

Comment on attachment 8441338 [details] [diff] [review]
Patch v5

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

Thanks!

Comment 12

[Florian Quèze [:florian]]

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.

Comment 13

[Florian Quèze [:florian]]

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?

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v6

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.

Comment 15

[(inactive) Nihanth Subramanya [:nhnt11]]

https://hg.mozilla.org/comm-central/rev/de14c1c218bc