Non-obvious error when calling job.fail(e)

[JProgrammer]

When calling job.fail() with an object of type Meteor.Error the code fails with the following stack trace.

I20160218-11:16:46.819(11)? Exception in setTimeout callback: Error: Match error: Expected plain object
I20160218-11:16:46.821(11)?     at check (packages/check/match.js:33:1)
I20160218-11:16:46.821(11)?     at JobCollectionBase._DDPMethod_jobFail (packages/vsivsi_job-collection/src/shared.coffee:1119:5)
I20160218-11:16:46.822(11)?     at JobCollection._methodWrapper (packages/vsivsi_job-collection/src/server.coffee:147:22)
I20160218-11:16:46.822(11)?     at packages/vsivsi_job-collection/src/server.coffee:85:40
I20160218-11:16:46.823(11)?     at methodCall (packages/vsivsi_job-collection/job/src/job_class.coffee:19:18)
I20160218-11:16:46.823(11)?     at Job.fail (packages/vsivsi_job-collection/job/src/job_class.coffee:823:14)
I20160218-11:16:46.823(11)?     at JobQueue.worker (server/jobs/collectPledges.js:51:21)
I20160218-11:16:46.824(11)?     at JobQueue._process (packages/vsivsi_job-collection/job/src/job_class.coffee:183:8)
I20160218-11:16:46.824(11)?     at [object Object]._.extend.withValue (packages/meteor/dynamics_nodejs.js:56:1)
I20160218-11:16:46.824(11)?     at withoutInvocation (packages/meteor/timers.js:6:1)

This is caused because the shared Job library just does a type check for Object with it is,

But in the shared.coffee of the jobs queue line 1119 it calls the meteor check library with check err, Object which is checking for a plain javascript object and fails giving the above stack trace and does not fail the job leaving it in running.

[vsivsi]

Hi, some of this may be obvious, but just so we're on the same page...

By "plain object" it means an object that can be serialized to EJSON. Meteor.Error objects are objects in Javascript (as in typeof new Meteor.Error("Woot!") == "object"), but are not serializable to EJSON, which is why the check is failing.

A job.fail() or any other DDP communicating method call can fail on the server-side, and the worker/client needs to check the return codes for errors. The worker Job sub-package tries to do a certain amount of sanity checking before sending off a DDP method call, but for security purposes the ultimate checks are done server-side, which can lead to cases like this.

The Job library is designed to run in a pure node.js (non-Meteor) environment (and be lightweight) to support workers running anywhere, so it isn't really feasible to perform all of the checks that the server does. Some of that checking comes free from the EJSON library itself (either on a Meteor client or using the npm ejson package).

EJSON would normally have caught this error on a client, however, from the stack trace it looks like your worker was actually running within the Meteor server process, which means that the request wasn't actually ever serialized to EJSON (for DDP on the wire), which would have provided an earlier "error" to a client/node.js worker. Instead your server-side worker was calling into the Fail method implementation directly, and so that failing check is really the first place this could have been caught.

The documentation for job.fail() does clearly state that the value for error needs to be an EJSONable object (or wrappable in one).

So yes, non-obvious, but to my eyes correct. If you have suggestions on how the documentation could make this clearer, I'd be happy to accept a PR!

Thanks.

[JProgrammer]

So a Meteor.Exception can be serialized with EJSON

> EJSON.stringify(new Meteor.Error("something-bad", "Really bad", {e: "stuff"}))
< "{"error":"something-bad","reason":"Really bad","details":{"e":"stuff"},"message":"Really bad [something-bad]","errorType":"Meteor.Error"}"

The error is coming because the object itself does not actually go through EJSON before this call. As you noted this is occuring during processJobs on my Meteor server.

A fix to the code that will work is instead of using check(e, Object)

Use

check(e, Match.Where(_.isObject));
e = EJSON.clone(e);

Sorry can only just read coffee don't know enough to write that correctly. This code keeps the dependencies in Meteor and does not increase it for the npm package. Use clone as it later adds the runId to the object when you don't want to modify the incoming object that was passed to job.fail()

[vsivsi]

Hi, I see what you are saying, and I didn't realize that Meteor supplied an EJSON custom type for Meteor.Error, although it makes sense. However, the "fix" above is too permissive and would allow all kinds of odd things through that check, for example:

// Does not trigger check with _.isObject() !!
job.fail(function () { console.log("Woot!")} ); 

The point of that code checking for a plain Object is to ensure that what you think you are storing is really transmittable by DDP and ultimately storable in MongoDB. Unfortunately there doesn't seem to be any obvious middle ground between the existing check and the one you propose.

One solution would be to eliminate (or make optional) the server-side shortcut that bypasses using Meteor.apply(...) to make local calls. That should route everything through EJSON even on the server, but at the cost of some server-side performance for what are now local calls. I think I would make this optional, because some projects probably depend on the current performance for server-side workers.

Another is to just document these cases for server-side and make clear that it is the developer's responsibility to convert everything into plain objects before passing them. job.save(), job.log() and job.done() presumably have similar behaviors when dealing with EJSONable but non-plain objects: https://github.com/vsivsi/meteor-job-collection/blob/master/src/shared.coffee#L57-L59 https://github.com/vsivsi/meteor-job-collection/blob/master/src/shared.coffee#L924 https://github.com/vsivsi/meteor-job-collection/blob/master/src/shared.coffee#L995

I need to think about this a bit more before proposing anything else. Feedback is welcomed.

[vsivsi]

If you would like to try bypassing the server side shortcut, you may do so by running:

Job._setDDPApply(Meteor.apply, jc.root);  // jc is your jobCollection object

I haven't tried this in reference to this issue but it should work and everything will go through meteor method calls with all that entails.