error handling is wrong in translator.js

Rex Huang

error log:
20/5 08:00 [179] - error: Could not parse email.json, syntax error? Skipping...
20/5 08:00 [179] - error: Error: Callback was already called.
at /opt/nodebb/node_modules/async/lib/async.js:30:31
at /opt/nodebb/node_modules/async/lib/async.js:251:21
at /opt/nodebb/src/emailer.js:45:6
at /opt/nodebb/public/src/modules/translator.js:160:6
at /opt/nodebb/public/src/modules/translator.js:180:4
at /opt/nodebb/public/src/modules/translator.js:226:6
at /opt/nodebb/public/src/modules/translator.js:289:5

Rex Huang

source code should be fixed

                    try {
                            callback(JSON.parse(data.toString()));
                    } catch (e) {
                            winston.error('Could not parse `' + filename + '.json`, syntax error? Skipping...');
                            callback({});    // this callback is unnecessary 
                    }

yariplus

@Rex-Huang There's nothing wrong with that part of the code.

When the JSON errors, it breaks out of the first callback call.
See Fiddle

This is the part that is erroring:

		keys.forEach(function(key) {
			translateKey(key, data, language, function(translated) {
				--count;
				if (count <= 0) {
					callback(translated.text);  // This is being called more than once because of '<=', maybe it should be '==='?
				}
			});
		});

Rex Huang

but the fact is it throw exception and nodebb crashed.

yariplus

@Rex-Huang Very true.

Could you post the contents of your email.json?

I typed some gibberish into mine and it caused the syntax error, but it did not cause it to crash.

Rex Huang

As my analysis, email.json is no problem, it's the bug in the plugin

frissdiegurke

@yariplus

It's not the JSON.parse failing here. Assuem JSON.parse works and the callback gets called with the result.
Now the callback throws an Error => callback gets called with {} again => Error: Callback was already called.
Besides the fact that through throwing Error within callback NodeBB should crash too, this would be better:

var json;
try {
  json = JSON.parse(data.toString());
} catch (e) {
  winston.error('Could not parse `' + filename + '.json`, syntax error? Skipping...');
  json = {};
}
callback(json);

since it ensures the correct error to be displayed.

EDIT: created PR: gh#3162

julian

Merged the commit into v0.7.0, thanks all

yariplus

@frissdiegurke Thanks, that makes sense. But I'm still curious, what could be causing the callback to crash?