{"action":"created","issue":{"url":"https://api.github.com/repos/18F/micropurchase/issues/123","labels_url":"https://api.github.com/repos/18F/micropurchase/issues/123/labels{/name}","comments_url":"https://api.github.com/repos/18F/micropurchase/issues/123/comments","events_url":"https://api.github.com/repos/18F/micropurchase/issues/123/events","html_url":"https://github.com/18F/micropurchase/issues/123","id":122287176,"number":123,"title":"Possible race condition with bid placement","user":{"login":"adelevie","id":86790,"avatar_url":"https://avatars.githubusercontent.com/u/86790?v=3","gravatar_id":"","url":"https://api.github.com/users/adelevie","html_url":"https://github.com/adelevie","followers_url":"https://api.github.com/users/adelevie/followers","following_url":"https://api.github.com/users/adelevie/following{/other_user}","gists_url":"https://api.github.com/users/adelevie/gists{/gist_id}","starred_url":"https://api.github.com/users/adelevie/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/adelevie/subscriptions","organizations_url":"https://api.github.com/users/adelevie/orgs","repos_url":"https://api.github.com/users/adelevie/repos","events_url":"https://api.github.com/users/adelevie/events{/privacy}","received_events_url":"https://api.github.com/users/adelevie/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":9,"created_at":"2015-12-15T14:44:27Z","updated_at":"2015-12-15T16:42:13Z","closed_at":null,"body":"It looks like we might have a race condition on our hands.\r\n\r\n### Problem\r\n\r\nIn order to successfully place a `Bid`, we check that it is valid. Validity means, among other things, that the bid amount is less than the current winning bid. In pseudocode:\r\n\r\n```ruby\r\ndef current_bid_amount\r\n  @auction.bids.last.amount\r\nend\r\n\r\n# BREAKPOINT\r\n\r\nif @bid.amount >= current_bid amount\r\n   raise UnauthorizedError, 'Bid amount must be less than the current bid.'\r\nend\r\n\r\n@bid.save\r\n```\r\n\r\nSuppose that after execution passes `# BREAKPOINT`, a user in another session successfully places a bid such that the value already returned in this session by `#current_bid_amount` is stale. This feels like a race condition to me.\r\n\r\n### Proposed Solution(s)\r\n\r\n#### Optimistic Locking + a new field on Auction + Transactions\r\n\r\nRead the [docs](http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html) on optimistic locking in ActiveRecord:\r\n\r\n> Optimistic locking allows multiple users to access the same record for edits, and assumes a minimum of conflicts with the data. It does this by checking whether another process has made changes to a record since it was opened, an ActiveRecord::StaleObjectError exception is thrown if that has occurred and the update is ignored.\r\n\r\nCurrently `Auction.has_many :bids`, but with locking, we should add a field (or fields) to `Auction` that represent the current winning bid. These fields could be a `current_winning_user_id`, an `current_winning_bid_amount`, and a `current_winning_bid_updated_at` timestamp. In pseudocode again:\r\n\r\n```ruby\r\n# Assumes optimistic locking is enabled for Auction\r\n\r\ndef current_bid_amount\r\n  @auction.current_winning_bid_amount\r\nend\r\n\r\nif @bid.amount >= current_bid amount\r\n   raise UnauthorizedError, 'Bid amount must be less than the current bid.'\r\nend\r\n\r\n@auction.current_winning_bid_amount = @bid.amount\r\n@auction.current_winning_user_id = @bid.bidder_id\r\n@auction.current_winning_bid_updated_at = DateTime.now\r\n\r\nif @auction.save\r\n  @bid.save\r\nend\r\n```\r\n\r\nNotice that we have two instances whose creation depends on the other. If `@auction` cannot save successfully, then we cannot create `@bid`. If `@bid` cannot be successfully created, the `@auction` update should be rolled back. To ensure consistency, we could use `ActiveRecord::Base::transaction`:\r\n\r\n```ruby\r\n# Assumes optimistic locking is enabled for Auction\r\n\r\ndef current_bid_amount\r\n  @auction.current_winning_bid_amount\r\nend\r\n\r\nif @bid.amount >= current_bid amount\r\n   raise UnauthorizedError, 'Bid amount must be less than the current bid.'\r\nend\r\n\r\nActiveRecord::Base.transaction do\r\n  @auction.current_winning_bid_amount = @bid.amount\r\n  @auction.current_winning_user_id = @bid.bidder_id\r\n  @auction.current_winning_bid_updated_at = DateTime.now\r\n\r\n  @auction.save\r\n  @bid.save\r\nend\r\n```\r\n\r\nWith this transaction, if `@auction.save` raises an `ActiveRecord::StaleObjectError` (due to optimistic locking--e.g. `@auction.current_winning_*` was updated in another session), the entire transaction will fail/be rolled back.\r\n\r\n@harrisj alluded to some problems with such a lock, mainly that if a process got slow, it would temporarily halt bidding for everyone. Let's discuss!\r\n\r\ncc @baccigalupi \r\n\r\nand the rest of our Rails pals @afeld, @jessieay, @ccostino, @yozlet, and @monfresh (who am I missing?).     "},"comment":{"url":"https://api.github.com/repos/18F/micropurchase/issues/comments/164822107","html_url":"https://github.com/18F/micropurchase/issues/123#issuecomment-164822107","issue_url":"https://api.github.com/repos/18F/micropurchase/issues/123","id":164822107,"user":{"login":"ajb","id":1270317,"avatar_url":"https://avatars.githubusercontent.com/u/1270317?v=3","gravatar_id":"","url":"https://api.github.com/users/ajb","html_url":"https://github.com/ajb","followers_url":"https://api.github.com/users/ajb/followers","following_url":"https://api.github.com/users/ajb/following{/other_user}","gists_url":"https://api.github.com/users/ajb/gists{/gist_id}","starred_url":"https://api.github.com/users/ajb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajb/subscriptions","organizations_url":"https://api.github.com/users/ajb/orgs","repos_url":"https://api.github.com/users/ajb/repos","events_url":"https://api.github.com/users/ajb/events{/privacy}","received_events_url":"https://api.github.com/users/ajb/received_events","type":"User","site_admin":false},"created_at":"2015-12-15T16:42:13Z","updated_at":"2015-12-15T16:42:13Z","body":"Actually, shit, I should correct myself here:\r\n\r\n> While you could always acquire a row lock on the auction record, this might be overkill... especially since I'm assuming that the majority of your requests involve a read to the auctions table.\r\n\r\nSince:\r\n\r\n> Row-level locks do not affect data querying; they block only writers and lockers to the same row.\r\n"}}