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

RESOLVED FIXED in 1.6

Status

Instantbird
Other
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nhnt11, Assigned: nhnt11)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8440261 [details] [diff] [review]
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.
Attachment #8440261 - Flags: review?(aleth)

Comment 2

3 years ago
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-
(Assignee)

Comment 3

3 years ago
Created attachment 8440808 [details] [diff] [review]
Patch v2
(Assignee)

Updated

3 years ago
Attachment #8440808 - Attachment description: movelogsweeper.patch → Patch v2
Attachment #8440808 - Flags: review?(aleth)
(Assignee)

Comment 4

3 years ago
Created attachment 8440810 [details] [diff] [review]
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.
Attachment #8440808 - Attachment is obsolete: true
Attachment #8440808 - Flags: review?(aleth)
Attachment #8440810 - Flags: review?(aleth)

Comment 5

3 years ago
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-
(Assignee)

Comment 6

3 years ago
Created attachment 8440879 [details] [diff] [review]
Patch v3

Address review comments.
Attachment #8440261 - Attachment is obsolete: true
Attachment #8440810 - Attachment is obsolete: true
Attachment #8440879 - Flags: review?(aleth)
(Assignee)

Comment 7

3 years ago
Created attachment 8441059 [details] [diff] [review]
Patch v4

Improved error handling, updated a comment.
Attachment #8440879 - Attachment is obsolete: true
Attachment #8440879 - Flags: review?(aleth)
Attachment #8441059 - Flags: review?(aleth)
(Assignee)

Comment 8

3 years ago
Created attachment 8441133 [details] [diff] [review]
Patch v4.01

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 9

3 years ago
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?
(Assignee)

Comment 10

3 years ago
Created attachment 8441338 [details] [diff] [review]
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)
Attachment #8441133 - Attachment is obsolete: true
Attachment #8441133 - Flags: review?(aleth)
Attachment #8441338 - Flags: review?(aleth)

Comment 11

3 years ago
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+
(Assignee)

Comment 14

3 years ago
Created attachment 8446872 [details] [diff] [review]
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.
Attachment #8441338 - Attachment is obsolete: true
Attachment #8446872 - Flags: review?(florian)

Updated

3 years ago
Attachment #8446872 - Flags: review?(florian) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 15

3 years ago
https://hg.mozilla.org/comm-central/rev/de14c1c218bc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.