Do we need memcache locks?

[blreams]

I was thinking some more about the race condition that was discussed in Issue #10. Basically, we had the following scenario:

Pooler P:

1. Gets the current pick sheet (call it revision 1).
2. Submits her picks based on pick sheet revision 1.

In parallel, Commissioner C:

1. Gets the current pick sheet (revision 1) and modifies it to revision 2.
2. Submits pick sheet revision 2 to the database.

Now these four events can interleave in six possible ways (there are 3 ways in which P1 occurs first and there are 3 ways in which C1 occurs first):

P1->P2->C1->C2:

• C2 must check for any successful P2 picks.
• C2 will not succeed.

P1->C1->P2->C2:

• C2 must check for any successful P2 picks.
• C2 will not succeed.

P1->C1->C2->P2:

• P2 must check for any successful C2 revision updates.
• P2 will not succeed.

C1->C2->P1->P2:

• Not a problem.
• P1 will get pick sheet revision 2.
• P2 will succeed.

C1->P1->C2->P2:

• P2 must check for any successful C2 revision updates.
• P2 will not succeed.

C1->P1->P2->C2:

• C2 must check for any successful P2 picks.
• C2 will not succeed.

I think the key to handling this correctly lies in treating memcache as the locking mechanism and doing the database updates after the lock is resolved by memcache.

I still need to think about this some more, though...would appreciate any insights that you have.

[jbholden]

This post addresses what I think the end behavior should be (comments in code boxes.)

P1->P2->C1->C2:

• C2 must check for any successful P2 picks.
• C2 will not succeed.

Brent:  The end result of this case would be that you live with the current spread value?

P1->C1->P2->C2:

• C2 must check for any successful P2 picks.
• C2 will not succeed.

Brent:  The end result of this case would be that you live with the current spread value?

P1->C1->C2->P2:

• P2 must check for any successful C2 revision updates.
• P2 will not succeed.

Brent:  The end result of this case would be that the spread change succeeds 
and when the pooler tries to enter their picks they get an error message say to repick.

C1->C2->P1->P2:

• Not a problem.
• P1 will get pick sheet revision 2.
• P2 will succeed.

Brent:  Agree.  No issue.

C1->P1->C2->P2:

• P2 must check for any successful C2 revision updates.
• P2 will not succeed.

Brent:  The end result of this case would be that the spread change succeeds 
and when the pooler tries to enter their picks they get an error message saying to repick.

C1->P1->P2->C2:

C2 must check for any successful P2 picks. C2 will not succeed.

Brent:  The end result of this case would be that you live with the current spread value?

[jbholden]

This post is a proposed solution (you may already be thinking like this with the memcache lock)

• Implement some sort of locking mechanism when the commissioner tries to change the spread value
• The extra step c_lock is show below in relevant cases (cases with Byron is from your original post, cases with Brent is the comments from Brent on the case)

code change summary (commissioner)

• commissioner sets a lock
• commissioner checks for any pooler submissions, if submissions, release lock and make no changes
• change spreads, submit new changes with later revision
• release lock

code change summary (pooler GET picks request)

• pooler makes GET request
• code checks for the commissioner lock, returns repick later message if lock is set
• if lock not set, return the current picks

code change summary (pooler POST picks)

• pooler makes POST request
• code checks for the commissioner lock, returns repick later message if lock is set
• if lock not set, check the submitted picks revision number (or time)
• if revision same, submit the picks
• if revision different, issue a repick message

P1->P2->C1->C2 (Byron)

• C2 must check for any successful P2 picks.
• C2 will not succeed.

P1->P2->C1->C2 (Brent)

• P1, then P2
• commissioner sets lock
• commissioner sees a pick has been submitted
• commissioner releases lock
• no change is made

P1->C1->P2->C2 (Byron)

• C2 must check for any successful P2 picks.
• C2 will not succeed.

\ adding a c_lock step indicating the setting of the lock

P1->C1->C_lock->P2->C2 (Brent)

• P1, C1
• commissioner sets lock
• P2 attempted, pooler receives a repick later message (pick code checks for the lock being set)
• C2
• release commissioner lock
• sometime later, pooler does P2

P1->C1->P2->C_lock->C2 (Brent)

• P1, C1, P2
• commissioner sets lock
• commissioner checks for P2 (P2 already occurred)
• release lock and live with changes (possible submission before lock set)

P1->C1->C2->P2 (Byron):

• P2 must check for any successful C2 revision updates.
• P2 will not succeed.

P1->C1->C2->P2 (Brent):

• P1
• C1, set lock, C2, release lock
• P2 gets a repick with new spread value message

C1->C2->P1->P2 (Byron):

• Not a problem.
• P1 will get pick sheet revision 2.
• P2 will succeed.

C1->C2->P1->P2 (Brent):

• C1, set lock, C2, release lock
• P1, P2 (no changes)

C1->P1->C2->P2 (Byron):

• P2 must check for any successful C2 revision updates.
• P2 will not succeed.

C1->C_lock->P1->C2->P2 (Brent version 1):

• C1
• set the lock
• P1 returns a repick later message (currently locked for changes)
• C2
• release lock
• P1, P2

C1->P1->c_lock->C2->P2 (Brent version 2):

• C1
• P1
• set the lock
• C2
• release lock
• P2 returns repick message

C1->P1->P2->C2 (Byron):

• C2 must check for any successful P2 picks.
• C2 will not succeed.

C1->c_lock->P1->P2->C2 (Brent version 1):

• C1
• set lock
• P1 gets repick later error message
• P2 should not occur (if it does it should be rejected)
• C2
• release lock

C1->P1->c_lock->P2->C2 (Brent version 2):

• C1
• P1
• set lock
• P2 returns repick later message
• C2
• release lock

[blreams]

I've been doing some more thinking about this and here is what I've come up with:

I'm going to add a new field to the Week table (probably call it block_picks and it will be a boolean). Basically, when this boolean is True, no picks will be accepted to the DB corresponding to that week. This will serve as the lock mentioned above. Anytime a pooler submits picks (either initial picks or edited picks), this boolean will be checked for the corresponding week. If block_picks is True, then the submitted picks will be rejected.

In Issue #6 we talked about having a week state (Invalid, Open, Locked). I'm not sure we need that anymore. The state is still a valid concept, but can be determined implicitly as follows:

1. Invalid (a week is invalid if it doesn't exist in the Week table)
2. Open (a week is open if it exists in the Week table and block_picks is False)
3. Locked (a week is locked if it exists in the Week table and block_picks is True)

There will be at least two cases where we set block_picks for a week:

1. A commissioner is submitting an edited pick sheet.
2. The pick deadline has passed and no more picks will be accepted for the given week.

For completeness, I've documented all the race conditions noted above in an excel spreadsheet. In my next commit of my development branch, I will include a top level "docs" directory and will add this spreadsheet.

[jbholden]

• Are you suggesting to get rid of the "lock_picks" datetime field and use the block_picks field instead?
• Does the commissioner have to manually lock the picks for the week?
• Or will there still be code to automatically lock the week after the deadline passes?

I have been thinking about the overall state of the pool for which I will open a new issue.