801665 - Provide error recovery for parser

Status: RESOLVED FIXED

(L20n :: JS Library, defect, P2)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

If L20n parser encounters a ParserError it should be able to recover and start again from the next closest entity.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: parser recovery patch

patch v1

Comment 2

[Staś Małolepszy :stas]

Comment on attachment 675058 [details] [diff] [review]
parser recovery patch

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

r=me with comments (see below).

::: lib/l20n.js
@@ +7,4 @@
>  
>    if (typeof exports !== 'undefined') {
>      L20n = exports;
> +    L20n.EventEmitter = require('./eemitter.js');

Can you call it 'events.js'?

@@ +199,5 @@
> +        parser.addEventListener('error', function(e) {
> +          debug('Parser error in ' + id);
> +          debug('Error msg is ' + e.msg);
> +          debug('Error around "' + e.context + '"');
> +        });

We should bubble an Exception up to the ctx here, I think.

if (env.DEBUG) {
  exception = new Exception(L20n.PARSE_ERROR, 'Non-fatal parser error in ' + id, e);
  _fire(fallbacks, [exception, true]);
}

The second argument, 'true', is responsible for calling the fallbacks in a no-op way.  

Can you check if this works as intended?  If it does, r=me.

@@ +208,3 @@
>          _fire(fallbacks, [exception]);
>          return false;
>        }

What happens if none of the entities defined in a resource is parsed correctly?  Do we just show debug output, or should we consider the resource as corrupt?  Sounds like bug 801663?

::: lib/parser.js
@@ +1,5 @@
>  (function() {
>    'use strict';
>  
> +  function Parser(L20n) {
> +    var emitter = new L20n.EventEmitter();

To avoid passing L20n to the Parser constructor, I think you could do the 'typeof exports' check before the Parser function definition and 'require' EventEmitter if needed, or check for 'this.L20n'.

It probably doesn't matter much, because bug 805163 will solve this anyways.

::: tests/lib/context.js
@@ +374,5 @@
>    });
>  
> +  describe('Broken lol', function(){
> +    before(function() {
> +      server.scenario = 'broken_lol';

Please remember to commit this LOL to lol-fixtures and update the submodule.

Comment 4

[Staś Małolepszy :stas]

Comment on attachment 675058 [details] [diff] [review]
parser recovery patch

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

Actually, I'm going to r- this because of the following line I just spotted.

::: lib/l20n.js
@@ +195,4 @@
>  
>      function parse(data) {
>        try {
> +        var parser = new L20n.Parser(L20n);

This doesn't look right.  We should create one parser per context.  In which case I'm not sure if we should have a single cache for downloaded files shared by all contexts.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

We should keep single cache for downloaded files but cache only if the LOL parses without errors.

Comment 6

[Staś Małolepszy :stas]

Or, cache the raw text, but not the AST, on error?

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch v2

updated patch:
 - comments addressed
 - optional parameter to define reaction to error

Comment 8

[Staś Małolepszy :stas]

Comment on attachment 678955 [details] [diff] [review]
patch v2

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

Sweet, r=me.  Let's file follow-up bugs about some of the issues we talked about today and that I mentioned below.

::: lib/l20n.js
@@ +194,4 @@
>      }
>  
>      function parse(data) {
> +      isDownloading = false;

This means that each resource file will be re-downloaded every time get() is called.  I understand why this is so, but can you please file a follow-up bug to make sure we fix how we call `parse` as a callback to `get` and `download`?

@@ +198,1 @@
>        try {

Can you move everything but the parse() call out of the try clause?

@@ +208,5 @@
> +            exception = new Exception(L20n.PARSE_ERROR, 'Non-fatal parser error in ' + id, e);
> +            _fire(fallbacks, [exception, true]);
> +          }
> +          hasErrors = true;
> +          // we should fire a fallback here to emit error in context?

Is this a question, or a comment? :)

@@ +222,4 @@
>          return elem.type == 'ImportStatement';
>        });
> +      if (!hasErrors) {
> +        ast = rast;

This suggests to me that hasError should be defined in the scope of the whole resource and be used in get() together with the check for `ast`.  No need to `rast` then.

Something like this maybe?

 this.get = function r_get(callback, fallback, async) {
      if (ast && !hasErrors) {
        callback(ast, imports);
      } else if (isCorrupted) {
        fallback(exception);
      } else {

@@ +596,4 @@
>        if (self.settings.locales && self.settings.locales.length > 1) {
>          subctx.settings.locales = self.settings.locales.slice(1);
>        } else {
> +        // this should throw "entity not available"

Mark it with XXX or TODO and file a bug please?

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Filed bugs, updated comments and pushed! Thanks :)

Comment 10

[Staś Małolepszy :stas]

https://github.com/l20n/l20n.js/commit/45b63a8ddd8d679dd390eeee7c2de5e569bd36a8