Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can I use synced-cron to time a single server event? #41

Closed
dalgard opened this issue Feb 23, 2015 · 18 comments
Closed

Can I use synced-cron to time a single server event? #41

dalgard opened this issue Feb 23, 2015 · 18 comments

Comments

@dalgard
Copy link

dalgard commented Feb 23, 2015

I'm building an auction system where the bidding on each item ends at a specific time. When an auction ends, certain operations should be executed. This is a one-time event, of course.

SyncedCron seems like the most robust solution for timed server events – is there an easy way of creating a job that runs once at a specific time in the future?

Thanks!

@dalgard
Copy link
Author

dalgard commented Feb 23, 2015

I've reearched a bit deeper.

It seems that synced-cron expects there to always be a next event on the Later.js object – however, in some cases, Later.js doesn't have any more events. It may be that a schedule is only be active between two points in time, or – as in my case – simply is a point in time:

SyncedCron.add({
  name: cron_name,
  schedule: function (parser) {
    // ending_at is a Date object set to some future date
    // there is no recurrence
    return parser.recur().on(ending_at).fullDate();
  },
  job: function () {
    console.log("hep!");
    // remove job so it doesn't recur
    SyncedCron.remove(cron_name);
  }
});

The above code results in the following exception:

Exception in setTimeout callback: TypeError: Cannot call method 'getTime' of undefined
     at scheduleTimeout (packages/percolate:synced-cron/synced-cron-server.js:240:1)

... since in the synced-cron code, the call to getTime fails when there are no more events. Is there a specific point in my code I could call SyncedCron.remove before the failing call is made?

diff = next[0].getTime() - now,

Other ways to make this work? Would be easy to patch synced-cron to stop when there are no next events, but I think this is an API decision.

dalgard added a commit to dalgard/meteor-synced-cron that referenced this issue Feb 23, 2015
@dalgard
Copy link
Author

dalgard commented Feb 23, 2015

So, I created a patch... What it does is simply removing the job if the schedule doesn't have any more events.

Also a minor improvement:

If the entry is removed inside its job function body, the next event isn't triggered. (The done variable is checked once more before calling _laterSetInterval.)

dalgard added a commit to dalgard/meteor-synced-cron that referenced this issue Feb 23, 2015
) – sorry, don't know anything about Tinytest
@tylerstraub
Copy link

I ran into a similar problem on one of my projects, I opted to go with the CRON parser (parser.cron) which accepts standard CRON syntax. From there I was able to formulate a single event using an expression such as; "10 18 26 4 ? 2015"

My apologies, I am now experiencing what everyone else on this thread is seeing. For some reason this method worked until I restarted my server. The meteor daemon is now crashing with my jobs scheduled in this way..

@zol
Copy link
Member

zol commented Apr 27, 2015

Thanks for the input @tylerstraub . My plan is still to take a look at this ticket as soon as I have some free time.

@dalgard
Copy link
Author

dalgard commented Apr 27, 2015

Don't know how much the code has developed, but my pull request solving this problem is quite simple – you could just use that ;) I'm using it in production, anyways.

@zol
Copy link
Member

zol commented Apr 28, 2015

@dalgard - The reason we ask that PR authors write tests is because we maintain quite a few o/s projects and don't necessarily have the bandwidth to go through each PR in detail and support it into the future. It's actually not that we're in doubt whether the code is production ready (we give you guys the benefit of doubt). If the PR comes with tests however, we're comfortable merging it with less scrutiny knowing that the author has described exactly what the functionality does and in fact we won't break it in future versions.

This way we can support functionality that we don't actually use ourselves as the tests become the vehicle that ensures we don't break stuff that others rely on.

In short, if we don't use something ourselves and there are no tests for it there is a pretty good chance that functionality may break in between releases - this is something we obviously want to avoid.

Hence with PR's like yours that don't come with tests, we need to schedule in the time and come up to speed with the functionality + write tests ourselves. Unfortunately, during busy times this falls down our priority queue.

@dalgard
Copy link
Author

dalgard commented May 3, 2015

I totally understand and earnestly consider it a healthy priority – had it been my project, I hope I would be equally methodical. All I would do, is encourage you to review my PR and see how limited the adjustments would be for resolving this issue.

Throwing together a patch on the basis of the lines I've submitted is a convenient path when professional obligations and other things take rightful precedence.

@BradRyan
Copy link

BradRyan commented Jun 9, 2015

Any update on this? Just noticed this issue on my project. It actually causes my server to crash and restart. In the meantime I'll see about adopting the solution that @dalgard came up with.

@BradRyan
Copy link

FYI there's an orphaned package which uses dalgard's solution. I've been using it... so far so good, but if a solution is settled on in the official package I'll be happy to switch back.
https://atmospherejs.com/orphan/synced-cron

@sylido
Copy link

sylido commented Jul 2, 2015

+1 for merge.

@rb365
Copy link

rb365 commented Jul 13, 2015

+1 for merge

@elie222
Copy link

elie222 commented Jul 19, 2015

+1 for tests to be added + merge

@davecromberge
Copy link

+1 for merge.

@KyleKing
Copy link

KyleKing commented Sep 5, 2015

+1 as well!

@zol
Copy link
Member

zol commented Sep 14, 2015

Released a fix to point in time schedules in version 1.3.0. Let me know if there are any issues with it. Thanks for your patience.

@yyl
Copy link

yyl commented May 19, 2016

I am using version 1.3.2, and want to fire a once-off job, but still facing the same issue:

    SyncedCron.add({
      name: 'reminder_email_' + uid,
      schedule: function(parser) {
        return parser.recur().on(new Date() + 60000).fullDate();
      },
      job: function() {
        // functions here
      }
    });

Console prints:

I20160519-17:56:27.539(-4)? Exception while invoking method 'addTask' TypeError: Cannot call method 'getTime' of undefined
I20160519-17:56:27.539(-4)?     at scheduleTimeout (packages/percolate_synced-cron/synced-cron-server.js:309:1)
I20160519-17:56:27.540(-4)?     at Object.SyncedCron._laterSetTimeout (packages/percolate_synced-cron/synced-cron-server.js:289:1)
I20160519-17:56:27.540(-4)?     at Object.SyncedCron._laterSetInterval (packages/percolate_synced-cron/synced-cron-server.js:257:1)
I20160519-17:56:27.540(-4)?     at scheduleEntry (packages/percolate_synced-cron/synced-cron-server.js:104:1)
I20160519-17:56:27.540(-4)?     at Object.SyncedCron.add (packages/percolate_synced-cron/synced-cron-server.js:127:1)
I20160519-17:56:27.541(-4)?     at [object Object].addTask (server/methods.js:107:20)
I20160519-17:56:27.541(-4)?     at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1704:12)
I20160519-17:56:27.541(-4)?     at packages/ddp-server/livedata_server.js:711:19
I20160519-17:56:27.541(-4)?     at [object Object]._.extend.withValue (packages/meteor/dynamics_nodejs.js:56:1)
I20160519-17:56:27.541(-4)?     at packages/ddp-server/livedata_server.js:709:40

@adrianbadarau
Copy link
Collaborator

adrianbadarau commented May 20, 2016

@yyl The cron is working as it should, the on method just requires a date not a timestamp.
The correct way to initialise the cron, if you just wanted to add 6s to the current date-time then you could just do something like:

schedule(parser){ let time = new Date(Date.now() + 6000); return parser.recur().on(time).fullDate(); }

@yyl
Copy link

yyl commented May 20, 2016

@adrianbadarau great that works, thanks!

ajlouie added a commit to ajlouie/meteor-synced-cron that referenced this issue Mar 18, 2018
…ed-cron

* 'master' of https://github.com/jamesgibson14/meteor-synced-cron:
  Adjusted versions
  Prevent _ensureIndex call from crashing an app
  Adjusted versions
  Fix E11000 duplicate key error collection
  Fixed rescheduling jobs when mongodb timed out during execution of job.
  Add comma so users can copy-paste without fixing syntax error
  Access to  job name when running the actual job
  Update README.md
  Release version 1.3.2 with dependency on Meteor 1.3
  Bump to version 1.3.1
  Remove the symlink in the example folder to prevent the symlink cycle error when building for Meteor 1.3
  Version 1.3.0
  Adds patch to later snippet to not throw on point in time schedules for percolatestudio#41
  Version bump
  Discard milliseconds from intendedAt
  One time scheduled events don't have a next occurrence - handle those gracefully
  small typo
  Added quotes to logged line when entry is removed.

# Conflicts:
#	.npm/package/npm-shrinkwrap.json
#	README.md
#	example/.meteor/.finished-upgraders
#	example/.meteor/release
#	example/.meteor/versions
#	package.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests