Multiple workers can execute the same scheduled job
madsodgaard opened this issue · 13 comments
Currently we create one ScheduledJobWorker per event loop. As far as I can tell, there is nothing in place to make sure that the job is only performed once and on one thread so that multiple workers do not execute the same job at the same time.
@madsodgaard I believe this is fixed in the latest beta. Please reopen if you see something different.
@mcdappdev - I am seeing this issue on the latest version. A ScheduledJob is executed multiple times (sometimes 5-6 times). Unfortunately this is to send out push notifications, so the issue is very visible... Can I set it up that only 1 worker executes the Scheduled jobs?
@rubencagnie sorry to hear this. Two questions:
- Are you running the scheduled worker in process?
- If so, how many instances of your app are you running?
@mcdappdev Sorry for the late response, but I have been trying to gather some more logs regarding this issue. Everyday, I have a Scheduled job to auto create some records in a database. It happens more than often that multiple instance are created. It's super hard to debug as it only happens sporatically. So I added a "Thread name" and "created_at" to each of my records (I pasted below) As you can see, the timestamps are within some milliseconds from each-other. Am I missing something?
thread = NIOThread(name = NIO-ELT-0-#7) - 2020-05-04 13:51:08.925791
thread = NIOThread(name = NIO-ELT-0-#8) - 2020-05-04 13:51:08.927348
thread = NIOThread(name = NIO-ELT-0-#5) - 2020-05-04 13:51:08.947569
thread = NIOThread(name = NIO-ELT-0-#3) - 2020-05-04 13:51:08.936349
thread = NIOThread(name = NIO-ELT-0-#7) - 2020-05-04 13:51:08.956550
thread = NIOThread(name = NIO-ELT-0-#7) - 2020-05-04 13:51:08.937938
thread = NIOThread(name = NIO-ELT-0-#10) - 2020-05-04 13:51:08.923284
thread = NIOThread(name = NIO-ELT-0-#3) - 2020-05-04 13:51:08.931963
thread = NIOThread(name = NIO-ELT-0-#8) - 2020-05-04 13:51:08.938529
My setup:
try app.queues.use(.redis(url: Environment.get("REDIS_URL") ?? "redis://127.0.0.1:6379"))
app.queues.schedule(VerifyDailyEvents())
.daily()
.at(5, 00, .am)
Answers to your question
- Yes, in process
- I just have 1 instance running, so that's concerning
@rubencagnie That's weird, but great job debugging - adding the NIOThread output was very smart.
@tanner0101 any ideas here? I thought we fixed this but the logs seem to indicate maybe not.
@mcdappdev - is there a way to limit the number of workers?
That's the part I'm not 100% sure on - I thought we limited it to only using one event loop at the library level but I guess not.
@rubencagnie how are you starting the scheduled jobs? You noted you're doing this in process, but where? Sharing that code would be helpful.
I'm looking at the code and I can't see anywhere obvious that the library could be doing this. We also have a test case for this: https://github.com/vapor/queues/blob/master/Tests/QueuesTests/QueueTests.swift#L114-L127
@tanner0101 - Thanks for sending the info
So, I might have found a lead. At a certain point in my code, I add a new ScheduledJob, but as that wasn't being triggered, I called req.application.queues.startScheduledJobs() again. Could that have been the cause? That each time that is called, the jobs are scheduled again? That way, the more I call that function, the more the scheduled job is executed?
Yes, that sounds like it. Each time you call startScheduledJobs it will start all of them without cancelling any previously started ones. There's currently no way to cancel already started jobs--we could consider adding API for that. But I'd recommend only calling startScheduledJobs once you've already configured all of them.
Great! I’ve deployed my change and I’ll let you know if it fixed it. I’d suggest closing this issue and I’ll comment back if it appears again. Thanks for all the help!
And to give some context on the usecase: we have events that are planned at a certain date. So when the server starts up, it schedules all these events to start (sending a push notification when it is started). But admins can also create events, so those need to be scheduled as well. If there was a way to “add” a scheduled event - that would be great...
@rubencagnie I think that could be tricky since scheduled jobs are stored in memory and not persisted like Redis. In the short term, one way to solve this would be to run a job every minute and check if a event should be dispatched - that obviously might be complex depending on your setup/usecase but it might work.