Agenda: Latest release 0.7.0 is broken

[mbadam-equinix]

Server.js code

var express = require('express');
var Agenda = require('agenda');
var agendaUI = require('agenda-ui');

var app = express();
var mongoConnectionString = "mongodb://localhost:27017/agenda";
var agenda = new Agenda({db: {address: mongoConnectionString,collection:'jobs',options:{}}});

agenda.define('Send Email', {priority: 'highest'}, function(job, done) {
    console.log('Send Email', job.attrs.data);
    setTimeout(done, 1000);
});
agenda.schedule('in 25 seconds', 'Send Email', {
    to: '[email protected]'
  , subject: 'Report'
});
agenda.start();
app.use('/agenda-ui', agendaUI(agenda, {poll: 1000}));
app.listen(9000);

A4370403:cloud-cron yc04535$ node server.js 
/Users/yc04535/i63/cloud/cloud-cron/node_modules/agenda/lib/agenda.js:277
    this._collection.insertOne(props, processDbResult);    // NF updated 22/04
                    ^
TypeError: Cannot read property 'insertOne' of undefined
    at Agenda.saveJob (/Users/yc04535/i63/cloud/cloud-cron/node_modules/agenda/lib/agenda.js:277:21)
    at Job.save (/Users/yc04535/i63/cloud/cloud-cron/node_modules/agenda/lib/job.js:226:15)
    at createJob (/Users/yc04535/i63/cloud/cloud-cron/node_modules/agenda/lib/agenda.js:209:9)
    at Agenda.schedule (/Users/yc04535/i63/cloud/cloud-cron/node_modules/agenda/lib/agenda.js:201:12)
    at Object.<anonymous> (/Users/yc04535/i63/cloud/cloud-cron/server.js:12:8)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
A4370403:cloud-cron yc04535$ vi package.json 
A4370403:cloud-cron yc04535$ vim server.js 
A4370403:cloud-cron yc04535$ node server.js 
/Users/yc04535/i63/cloud/cloud-cron/node_modules/agenda/lib/agenda.js:277

[loris]

+1

/app/node_modules/agenda/lib/agenda.js:271
    this._collection.findAndModify({name: props.name, type: 'single'}, {}, update, {upsert: true, new: true}, processDbResult);
                    ^

TypeError: Cannot read property 'findAndModify' of undefined
    at Agenda.saveJob (/app/node_modules/agenda/lib/agenda.js:271:21)
    at Job.save (/app/node_modules/agenda/lib/job.js:226:15)
    at createJob (/app/node_modules/agenda/lib/agenda.js:186:9)
    at Agenda.every (/app/node_modules/agenda/lib/agenda.js:176:12)
    at Object.<anonymous> (/app/lib/agenda.js:68:8)
    at Module._compile (module.js:435:26)
    at Object.Module._extensions..js (module.js:442:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:311:12)
    at Module.require (module.js:366:17)

[loris]

Apparently some breaking changes was committed (0.7.0) last week. Release number should have been 1.0.0 or something.
agenda.start() (and any other calls doing mongo requests like agenda.every etc.) should now be delayed until callback from new Agenda(config, callback); is fired. Otherwise db_init is executed too late.

[loris]

I ended up using event emitter this way:

var agenda = new Agenda({
  db: { address: process.env.MONGOLAB_URI, collection: 'jobs' }
}, function(err) {
  if (err) {
    console.log(err);
    throw err;
  }
  agenda.emit('ready');
  agenda.start();
});

And wrapped my calls to agenda into agenda.on('ready', function(){ });
Maybe this is an event that should be added into agenda codebase.

[patrickfatrick]

@loris Good call on wrapping it all in a 'ready' event. That fixed it for me.

[jdiamond]

The 'ready' event is not getting emitted when using the mongo option instead of db. I agree with @loris. Backwards compatibility was seriously broken and the version should indicate that.

[loris]

@jdiamond: it is not emitted because it is not needed (when using mongo, you provide your own and already opened mongo connection), maybe this could be added to README.md
Otherwise issue can be closed.

[jdiamond]

It makes the API inconsistent. If it was emitted in all cases, it wouldn't have to be called out in the README as an exception.

[loris]

@jdiamond You right, opened another PR (https://github.com/rschmukler/agenda/pull/219), feel free to test it, i’m not using config.mongo in my setup.

[spamguy]

Running node 4.2.1, Agenda 0.7.2. On app start:

/Users/spamguy/src/diplomacy/node_modules/agenda/lib/agenda.js:341
  if ( this._mdb.s.topology.connections().length === 0 ) {
                ^

TypeError: Cannot read property 's' of undefined
    at Agenda._findAndLockNextJob (/Users/spamguy/src/diplomacy/node_modules/agenda/lib/agenda.js:341:17)
    at jobQueueFilling (/Users/spamguy/src/diplomacy/node_modules/agenda/lib/agenda.js:420:10)
    at processJobs (/Users/spamguy/src/diplomacy/node_modules/agenda/lib/agenda.js:404:7)
    at doNTCallback0 (node.js:417:9)
    at process._tickCallback (node.js:346:13)
    at Function.Module.runMain (module.js:469:11)
    at startup (node.js:134:18)
    at node.js:961:3

[loris]

@spamguy are you wrapping your calls inside the ready event? do you have a code sample to show?

[Zertz]

I have Agenda 0.7.2 running fine on Node 4.2.1, it's probably the ready event biting you.

Great release, but it should have been 1.0.0. There are also some edge cases where processDbResult can throw because of the new MongoDB driver.

[3choBoomer]

I found an issue which may or may not be related to the issues you guys are seeing.

self vs. this in the refactored code done by @liamdon.

https://github.com/rschmukler/agenda/pull/223

[spamguy]

@loris: You're right, there was no ready() statement. I definitely got by without them prior to 0.7. Is there an upgrade guide to emphasize this need?

[niftylettuce]

Why is this still not fixed? Why is there no supporting documentation for a possible fix?

[niftylettuce]

This is clearly broken, and nothing is patched to support mongo usage with agenda-ui

[niftylettuce]

Nevermind, we just need to get this PR merged https://github.com/moudy/agenda-ui/pull/17

[niftylettuce]

Nevermind, it still doesn't work. The docs are outdated or something. Or this thing is broken.

Error received: Cannot read property 'insertOne' of undefined

Usage:

var Agenda = require('agenda');
var config = {
  agenda: {
    db: {
      address: mongoURI,
      collection: 'jobs'
    }
  }
};
var agenda = new Agenda(config.agenda);
agenda.schedule('now', 'post recurring stocks'); // <-- error thrown here
agenda.schedule('in 5 minutes', 'post recurring stocks');

Version: ^0.7.3

[niftylettuce]

The fix was as follows:

-agenda.schedule('now', 'post recurring stocks'); // <-- error thrown here
-agenda.schedule('in 5 minutes', 'post recurring stocks');
+agenda.on('ready', function() {
+  agenda.schedule('now', 'post recurring stocks');
+  agenda.schedule('in 5 minutes', 'post recurring stocks');
+});

[niftylettuce]

@rschmukler Perhaps we should update the documentation and also throw a better error than the one I commented about here? https://github.com/rschmukler/agenda/issues/213#issuecomment-161131033

[loris]

@niftylettuce this has been fixed and properly documented a month ago, you need to wrap to wait for the ready event before using agenda (see #215)

[niftylettuce]

@loris The error thrown should say maybe say this instead?

Connection not ready yet, please wrap your schedule calls with ready() event, see documentation at https://github.com/rschmukler/agenda for more info

I just see people making this error and thinking its a problem with their collection or MongoDB instance or something. The error received here is not clear to the situation.

[loris]

That's a good idea, feel free to PR. An even better (but harder to implement) would be to queue any calls to the schedule method (et al) and dispatch them once the mongo connection is ready.
In the meantime, using the README should be fine.

[helsont]

+1 niftylettuce on 'ready' fix

[mathieumg]

I still get the error mentioned in https://github.com/rschmukler/agenda/issues/213#issuecomment-151718707

I'm on 0.7.6 and the Mongo server is on 3.0.7. In https://github.com/rschmukler/agenda/blob/52ea37fb1cb7016e7d8616b4e057edaff7efaac0/lib/agenda.js#L380 s doesn't exist on this._mdb but it does on this._mdb.db and there is also a topology property on this._mdb. Also see https://github.com/rschmukler/agenda/issues/242

[Zertz]

@rschmukler I'm not sure how much it would help, but perhaps (re)releasing the current version as 1.0 and creating a changelog would help alleviate this "issue".

The problems stems from the fact that, even though it is documented, a breaking change was introduced in a semver minor.

@everyone in this thread having issues: read the docs!

[loris]

@Zertz @rschmukler The solution I pulled and documented (wrapping your calls inside the ready event) was supposed to be a temporary fix. People still complaining here about the errors show we can’t just tell them to read the docs. I will try to find some time this week to make another PR, but I think the proper solution would be to have the wrapping done _inside_ the library. For instance changing the now method from:

Agenda.prototype.now = function(name, data, cb) {
  if (!cb && typeof data == 'function') {
    cb = data;
    data = undefined;
  }
  var job = this.create(name, data);
  job.schedule(new Date());
  job.save(cb);
  return job;
};

to

Agenda.prototype.now = function(name, data, cb) {
  if (!cb && typeof data == 'function') {
    cb = data;
    data = undefined;
  }
  var job = this.create(name, data);
  job.schedule(new Date());
  this.on('ready', function() {
    job.save(cb);
  });
  return job;
};

will make use of agenda.now not throws any errors. What do you think?

[Zertz]

I'll have a look at the code, but the solution is probably a callback. Either way, the fix is a breaking change and it must be published under 1.0

[loris]

Maybe I was not clear enough, but the proposed solution is not a breaking change, it would make pre-0.7.0 code using agenda works without throwing the current error. Instead of asking agenda users to delay their use of public methods until mongodb connection is properly opened (what the ready wrapping is meant for), the library should silently enqueue these calls and process them once the connection is OK.

[mathieumg]

Regarding https://github.com/rschmukler/agenda/issues/213#issuecomment-172279273 turns out I was passing Mongoose's Mongoose.connection instead of Mongoose.mongo as the mongo setting to Agenda, so that was my bad.

[Zertz]

@loris Sorry I was on my phone and misread the code. Wrapping ready internally works as a patch to 0.7, but I think there is a better long term fix.

The real issue stems from this particular callback. It seems to me that the error and ready events are doing what is expected to be the callback's purpose in Node. Connecting to a database is inherently async so (IMHO) the callback should not be optional. My expectation as a Node developer would be to write the initialization code like this:

const agenda = new Agenda({
  ...
}, (err, collection) => {
  // Define jobs
  // Create job start, success, error and complete events
})

JusI to be clear, I think your fix is the way to go for 0.7. As for my proposed solution, I am open to submitting a PR upon agreement.

[loris]

@Zertz I’m not sure your solution would be convenient. How would you organize your jobs in different files? Wrapping ready internally would make it seamless for the developper: no need to wait for an event or use callback.
It is quite like is working Mongoose. You can have mongoose queries (User.find({}, (err, user) => ...);) in various files in your code, no matter when they are loaded, they won’t throw any error, their execution will be automatically postponed internally by mongoose until the mongodb connection is ready. It would not be convenient at all to have to wrap all our mongoose queries inside callbacks or event handlers. (From the mongoose README : Important! Mongoose buffers all the commands until it's connected to the database. This means that you don't have to wait until it connects to MongoDB in order to define models, run queries, etc.)
Plus it would make it backward-compatible with 0.6.x

[jdiamond]

Is the burden/complexity of buffering commands until the connection is ready worth it? Since backwards compatibility is already broken, my opinion is no.

However, I would like to propose breaking compatibility one more time before 1.0.0 to make connecting as explicit as possible. The current API is too subtle. It's not obvious that you have to wait for an event before using the object you just constructed.

What I mean is: Given the correct options, Agenda instances immediately connect to the database as soon as they're constructed. I was very surprised/annoyed by this. It doesn't give me any opportunity to set more options, override methods, etc, like I can with more typical JavaScript objects. Very much like how net.Socket or net.Server don't actually do anything after constructing until you call .connect() or .listen().

I would prefer some sort of explicit method that makes it super obvious what is happening and gives users a chance to use a callback or promise to know when the connection is ready. This is similar to what @Zertz proposed, except doing it with a method and not the constructor. We can still support the ready event, but I think most people would prefer to use the callback or promise.

Here is an example:

var myAgenda = new Agenda({ /* options */ });

// set more options, override methods, etc
// not connected to any database yet

// now we connect with a callback:
myAgenda.connect(function(err) {
    // schedule jobs here
});

// or with a promise:
myAgenda.connect().then(function() {
    // schedule jobs here
});

// or use the event:
myAgenda.connect();
myAgenda.on('ready', function() {
    // schedule jobs here
});

[mathieumg]

I think the above proposal by @jdiamond is the way to go.

[Zertz]

@jdiamond I like this idea as well. It makes a lot more sense than my proposition of a constructor with a callback.

@loris I am very much neutral toward providing a fix for 0.7, it depends how much effort one is willing to put into a version that has already broken compatibility.

[Zertz]

@jdiamond if we want promises, they should be implemented in the entire API though, otherwise it gets confusing as to which feature you can then() and which only accepts a callback.

[jdiamond]

@Zertz Agreed. Supporting promises can always be added later without breaking backwards compatibility.

[snowfrogdev]

Hey guys, just wondering what the status was on this. I seem to be having a related issue. See #435.

[simison]

Examples now have agenda.on('ready, ... in them so I suppose this can be closed.

[simison]

@jdiamond would be really lovely if you could do a PR for adding promises! See #414 & #441