percolatestudio / meteor-migrations

Simple migration system for Meteor
https://atmospherejs.com/percolate/migrations
MIT License
245 stars 58 forks source link

lock is not atomic #37

Closed meonkeys closed 9 years ago

meonkeys commented 9 years ago

setLocked should make sure a document was actually updated. If not, another app running the same code connecting to the same database may have already run the migration.

zol commented 9 years ago

I think I can guess what you mean but I'm not entirely sure just from the description. It's been on my mental list for a while to use the same locking technique in migrations as we did for synced-cron here: https://github.com/percolatestudio/meteor-synced-cron/blob/master/synced-cron-server.js#L200-L213

Is that what you meant?

meonkeys commented 9 years ago

Yep, exactly!

Notes to self / posterity follow. I wanted to jot down an atomic upsert I did in the MongoDB shell. I think doing an atomic update or insert from Meteor does indeed need a try/catch as with synced-cron.

Any atomic MongoDB operation will provide meteor-migrations a usable mutually-exclusive lock, since (it appears) a core assumption of meteor-migrations is that there is one central MongoDB database in use.

update is atomic. Here's an example in the mongo shell:

> db.locks.find()
{
  "_id": "control",
  "version": 1,
  "locked": false
}

> db.locks.update({_id:"control"},{$set:{version:1,locked:true}},{upsert:true})
Updated 1 existing record(s) in 6ms
WriteResult({
  "nMatched": 1,
  "nUpserted": 0,
  "nModified": 1
})

{"nModified": 1} means we just got the lock. Any others trying to obtain the lock at the same moment get {"nModified": 0}:

> db.locks.update({_id:"control"},{$set:{version:1,locked:true}},{upsert:true})
Updated 1 existing record(s) in 2ms
WriteResult({
  "nMatched": 1,
  "nUpserted": 0,
  "nModified": 0
})
zol commented 9 years ago

Oh cool, that's a neat trick. Want to create a PR for it?

meonkeys commented 9 years ago

Sure, I'll give it a shot!