Skip to content

Add table lock to prevent race conditions#13

Closed
ajb wants to merge 3 commits intoderrickreimer:masterfrom
ajb:monogamy
Closed

Add table lock to prevent race conditions#13
ajb wants to merge 3 commits intoderrickreimer:masterfrom
ajb:monogamy

Conversation

@ajb
Copy link
Copy Markdown
Contributor

@ajb ajb commented Jun 2, 2015

This is probably not ideal for the core library, but I wanted to open this PR for continued discussion of #12. I first tried to wrap #set_sequential_id in an advisory lock (using https://github.com/mceachen/with_advisory_lock), but there was a less-frequent race condition where a different row would be created in between #set_sequential_id and #save being called.

@ajb ajb mentioned this pull request Jun 2, 2015
@derrickreimer
Copy link
Copy Markdown
Owner

@ajb thanks for submitting this, I'll take a look

@ajb
Copy link
Copy Markdown
Contributor Author

ajb commented Aug 27, 2015

FWIW, we've been using this in production for a while now.

@hakunin
Copy link
Copy Markdown

hakunin commented Nov 4, 2015

This is how its done - locking records.
https://coderwall.com/p/h0qura/scope-sequences-in-activerecord
http://stackoverflow.com/questions/17024447/add-auto-increment-with-scope-to-existing-column-in-migration-file-rails

The first link is based on this gem.

I think it should be noted that this gem is not thread safe in the readme, since without thread safeti its pretty much useless for anyone.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't guarantee race-free behavior as far as I can tell.

It will accidentally guarantee race-free behavior only on Postgres because locks are always held open until the end of the transaction. However it only appears to guarantee race-free behavior on MySQL, in actual fact it suffers from the following problem:

Consider this scenario:

  • Thread 1 locks the table and uses this method to get the next id in the sequence.
  • Thread 1 releases the lock.

CONTEXT SWITCH

  • Thread 2 locks the table and uses this method to get the next id in the sequence.
  • Thread 2 releases the lock.
  • Thread 2 inserts successfully.

CONTEXT SWITCH

  • Thread 1 attempts to insert with the same value that thread 2 just used.

Result: DISASTER

I think it is more confusing to falsely appear to guarantee race-free behavior on MySQL than to explicitly state that it only works with the postgres adapter.

See my PR here for a postgres-specific solution with working tests #16 .

@ajb ajb closed this Nov 24, 2015
@ajb
Copy link
Copy Markdown
Contributor Author

ajb commented Nov 24, 2015

Thanks for the review! Closing in favor of #16, which looks 🌟

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

Successfully merging this pull request may close these issues.

4 participants