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

Work around race condition on last_seen_at update #674

Merged
merged 1 commit into from May 27, 2018
Merged

Work around race condition on last_seen_at update #674

merged 1 commit into from May 27, 2018

Conversation

glebm
Copy link
Collaborator

@glebm glebm commented Jan 10, 2018

There is race condition in code like:

# Say this doesn't find a user and initializes a new one:
user_detail = Thredded::UserDetail.find_or_initialize_by(user_id: user_id)
# A user gets created by another thread / process in-between these two lines:
user_detail.update!(last_seen_at: now)
# ActiveRecord::RecordNotUnique is raised

Our two options here are to either rescue the exception and retry, or use upsert. Unfortunately, there is no suitable upsert gem for Ruby that I could find.

https://github.com/seamusabshere/upsert is the most promising one but it currently has a number of show-stopping issues, namely it doesn't let us specify "insert but not update nor select" columns manually, and has potential thread-safety issues. May revisit this in the future.

There is race condition in code like:

```ruby
user_detail = Thredded::UserDetail.find_or_initialize_by(user_id: user_id)
user_detail.update!(last_seen_at: now)
```

Our two options here are to either rescue the exception and retry, or
use upsert. Unfortunately, there is no suitable upsert gem for Ruby that
I could find.

https://github.com/seamusabshere/upsert is the most promising one but it
currently has a number of show-stopping issues, namely it doesn't let us
specify "insert but not update nor select" columns manually, and has
potential thread-safety issues. May revisit this in the future.
@glebm glebm added the bug label May 27, 2018
@glebm glebm merged commit ce28d97 into 0-15-dev May 27, 2018
@glebm glebm deleted the upsert branch May 27, 2018 08:27
@timdiggins
Copy link
Collaborator

Hi Gleb 👋 -- for what it's worth this must have been a real observable problem -- I know because I've got an issue upgrading a real database to this version of Thredded, because the index can't be created 😔.

I've made a crappy little addition to my migration to delete the offending unique keys which I post here in case useful to anyone else / me later on.

    subselect = <<-SQL
    SELECT tmu.id
    FROM thredded_messageboard_users tmu, thredded_messageboard_users tmu2
    WHERE tmu.thredded_user_detail_id = tmu2.thredded_user_detail_id AND tmu.thredded_messageboard_id = tmu2.thredded_messageboard_id AND tmu.id > tmu2.id
    SQL
    loop do
      break unless Thredded::MessageboardUser.where("id in (#{subselect})").delete_all > 0
    end

(based on https://stackoverflow.com/a/35990071/109175 -- should work on mysql as well as postgresql. I only encountered the problem with Thredded::MessageboardUser (probably because there are many more of them for me -- few users, many messageboards)

@glebm
Copy link
Collaborator Author

glebm commented Oct 10, 2018

Hi! I also had this error when running the migration! I used slightly different code to delete the duplicates: https://github.com/thredded/thredded/releases/tag/v0.15.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants