michaelklishin / quartz-mongodb

A MongoDB-based store for the Quartz scheduler. This fork strives to be as feature complete as possible. Originally by MuleSoft.
Other
249 stars 203 forks source link

RuntimeException no more #51

Closed lordbuddha closed 10 years ago

lordbuddha commented 10 years ago

Seeing low (relatively) levels of this exception in production.

2013-12-13 14:01:27,147 53567111 ERROR [CJS] QuartzSchedulerThread - quartzSchedulerThreadLoop: RuntimeException no more java.util.NoSuchElementException: no more at com.mongodb.DBApiLayer$Result.next(DBApiLayer.java:384) at com.mongodb.DBApiLayer$Result.next(DBApiLayer.java:346) at com.mongodb.DBCursor._next(DBCursor.java:421) at com.mongodb.DBCursor.next(DBCursor.java:494) at com.novemberain.quartz.mongodb.MongoDBJobStore.removeTrigger(MongoDBJobStore.java:205) at com.novemberain.quartz.mongodb.MongoDBJobStore.doAcquireNextTriggers(MongoDBJobStore.java:550) at com.novemberain.quartz.mongodb.MongoDBJobStore.acquireNextTriggers(MongoDBJobStore.java:485) at org.quartz.core.QuartzSchedulerThread.run(QuartzSchedulerThread.java:264)

Looking at the code, it seems that the following

  public boolean removeTrigger(TriggerKey triggerKey) throws JobPersistenceException {
    BasicDBObject dbObject = Keys.keyToDBObject(triggerKey);
    DBCursor triggers = triggerCollection.find(dbObject);
    if (triggers.count() > 0) {
      DBObject trigger = triggers.next();
      if (trigger.containsField(TRIGGER_JOB_ID)) {
        // There is only 1 job per trigger so no need to look further.
        DBObject job = jobCollection.findOne(new BasicDBObject("_id", trigger.get(TRIGGER_JOB_ID)));
        // Remove the orphaned job if it's durable and has no other triggers associated with it,
        // remove it
        if (!job.containsField(JOB_DURABILITY) || job.get(JOB_DURABILITY).toString().equals("false")) {
          DBCursor referencedTriggers = triggerCollection.find(new BasicDBObject(TRIGGER_JOB_ID, job.get("_id")));
          if (referencedTriggers != null && referencedTriggers.count() <= 1) {
            jobCollection.remove(job);
          }
        }
      } else {
        log.debug("The trigger had no associated jobs");
      }
      triggerCollection.remove(dbObject);

      return true;
    }

    return false;
  }

should / could be changed to

  public boolean removeTrigger(TriggerKey triggerKey) throws JobPersistenceException {
    BasicDBObject dbObject = Keys.keyToDBObject(triggerKey);
    List<DBObject> triggers = triggerCollection.find(dbObject).limit(2).toArray();
    if (triggers.size() > 0) {
      DBObject trigger = triggers.get(0);
      if (trigger.containsField(TRIGGER_JOB_ID)) {
        // There is only 1 job per trigger so no need to look further.
        DBObject job = jobCollection.findOne(new BasicDBObject("_id", trigger.get(TRIGGER_JOB_ID)));
        // Remove the orphaned job if it's durable and has no other triggers associated with it,
        // remove it
        if (job != null && (!job.containsField(JOB_DURABILITY) || job.get(JOB_DURABILITY).toString().equals("false"))) {
          List<DBObject> referencedTriggers = triggerCollection.find(new BasicDBObject(TRIGGER_JOB_ID, job.get("_id"))).limit(2).toArray();
          if (referencedTriggers.size() == 1) {
            jobCollection.remove(job);
          }
        }
      } else {
        log.debug("The trigger had no associated jobs");
      }
      triggerCollection.remove(dbObject);

      return true;
    }

    return false;
  }

The reason is that .count() does not indicate the number of results the cursor has but rather what the DB currently has... hence the possibility for this error to occur. Using .limit(2).toArray() and .size() operates solely on the cursor and thus hopefully preclude this from happening.

Can update my fork and issue a pull if this is agreed.

lordbuddha commented 10 years ago

Also seeing low levels of a null pointer exception on line 211.... i.e. if the job is not found so an additional check is required for that:-

  public boolean removeTrigger(TriggerKey triggerKey) throws JobPersistenceException {
    BasicDBObject dbObject = Keys.keyToDBObject(triggerKey);
    DBCursor triggers = triggerCollection.find(dbObject).limit(2);
    if (triggers.length() == 1) {
      DBObject trigger = triggers.next();
      if (trigger.containsField(TRIGGER_JOB_ID)) {
        // There is only 1 job per trigger so no need to look further.
        DBObject job = jobCollection.findOne(new BasicDBObject("_id", trigger.get(TRIGGER_JOB_ID)));
        // Remove the orphaned job if it's durable and has no other triggers associated with it,
        // remove it
        if (job != null && (!job.containsField(JOB_DURABILITY) || job.get(JOB_DURABILITY).toString().equals("false"))) {
          DBCursor referencedTriggers = triggerCollection.find(new BasicDBObject(TRIGGER_JOB_ID, job.get("_id"))).limit(2);
          if (referencedTriggers.length() == 1) {
            jobCollection.remove(job);
          }
        }
      } else {
        log.debug("The trigger had no associated jobs");
      }
      triggerCollection.remove(dbObject);

      return true;
    }

    return false;
  }